Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On 2019-10-30 14:40, Michael Niedermayer wrote: On Tue, Oct 29, 2019 at 04:39:16PM +0300, Andrey Semashev wrote: On 2019-10-26 14:05, Andrey Semashev wrote: The decoder never marks pictures as I-frames, which results in no keyframe indication and incorrect frame skipping, in cases when keyframes should be decoded. This commit works around this decoder limitation and marks I-frames and keyframes based on "freeze picture release" bit in h261 picture header. This reflects h261enc behavior. So, is this patch acceptable? If yes, could someone merge it please? The patch does 2 things 1. it skips !freeze_picture_release frames if the user wants non key frames skiped that seems reasonable 2. it marks freeze_picture_release frames as key/intra frames i do not know if this is a good idea as i do not know if every encoder sets this on key frames. Our encoder is not every encoder If the patch intstead would set the flag depending on the blocks then i would apply it otherwise it would be nice to see a bit more evidence that this flag is really always set for keyframes by most encoders. Or maybe someone "knows" this then it can be applied too. So I cant awnser if the patch is acceptable as is as i do not know from the information provided how widely this flag is set on keyframes Obviously, I cannot say that every encoder in the world sets freeze_picture_release on keyframes. I quoted the H.261 spec that says that that should be the case and h261enc follows it. I cannot provide any more solid evidence. PS: Note that skipping non-keyframes and marking frames as keyframes should be related. You can't accept the skipping part but not the marking part. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On Tue, Oct 29, 2019 at 04:39:16PM +0300, Andrey Semashev wrote: > On 2019-10-26 14:05, Andrey Semashev wrote: > >The decoder never marks pictures as I-frames, which results in no > >keyframe indication and incorrect frame skipping, in cases when > >keyframes should be decoded. > > > >This commit works around this decoder limitation and marks I-frames > >and keyframes based on "freeze picture release" bit in h261 picture > >header. This reflects h261enc behavior. > > So, is this patch acceptable? If yes, could someone merge it please? The patch does 2 things 1. it skips !freeze_picture_release frames if the user wants non key frames skiped that seems reasonable 2. it marks freeze_picture_release frames as key/intra frames i do not know if this is a good idea as i do not know if every encoder sets this on key frames. Our encoder is not every encoder If the patch intstead would set the flag depending on the blocks then i would apply it otherwise it would be nice to see a bit more evidence that this flag is really always set for keyframes by most encoders. Or maybe someone "knows" this then it can be applied too. So I cant awnser if the patch is acceptable as is as i do not know from the information provided how widely this flag is set on keyframes thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On 2019-10-26 14:05, Andrey Semashev wrote:
The decoder never marks pictures as I-frames, which results in no
keyframe indication and incorrect frame skipping, in cases when
keyframes should be decoded.
This commit works around this decoder limitation and marks I-frames
and keyframes based on "freeze picture release" bit in h261 picture
header. This reflects h261enc behavior.
So, is this patch acceptable? If yes, could someone merge it please?
---
libavcodec/h261.h| 1 +
libavcodec/h261dec.c | 27 ++-
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/libavcodec/h261.h b/libavcodec/h261.h
index 399a404b2b..6662d38d6d 100644
--- a/libavcodec/h261.h
+++ b/libavcodec/h261.h
@@ -37,6 +37,7 @@
typedef struct H261Context {
MpegEncContext s;
+int freeze_picture_release; // 1 if freeze picture release bit is set in the picture header
int current_mba;
int mba_diff;
int mtype;
diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c
index 14a874c45d..3b1711a21d 100644
--- a/libavcodec/h261dec.c
+++ b/libavcodec/h261dec.c
@@ -502,9 +502,9 @@ static int h261_decode_picture_header(H261Context *h)
s->avctx->framerate = (AVRational) { 3, 1001 };
/* PTYPE starts here */
-skip_bits1(>gb); /* split screen off */
-skip_bits1(>gb); /* camera off */
-skip_bits1(>gb); /* freeze picture release off */
+skip_bits1(>gb); /* split screen indicator */
+skip_bits1(>gb); /* document camera indicator */
+h->freeze_picture_release = get_bits1(>gb); /* freeze picture release */
format = get_bits1(>gb);
@@ -532,7 +532,8 @@ static int h261_decode_picture_header(H261Context *h)
/* H.261 has no I-frames, but if we pass AV_PICTURE_TYPE_I for the first
* frame, the codec crashes if it does not contain all I-blocks
- * (e.g. when a packet is lost). */
+ * (e.g. when a packet is lost). We will fix the picture type in the
+ * output frame based on h->freeze_picture_release later. */
s->pict_type = AV_PICTURE_TYPE_P;
h->gob_number = 0;
@@ -590,6 +591,7 @@ static int h261_decode_frame(AVCodecContext *avctx, void
*data,
H261Context *h = avctx->priv_data;
MpegEncContext *s = >s;
int ret;
+enum AVPictureType pict_type;
AVFrame *pict = data;
ff_dlog(avctx, "*frame %d size=%d\n", avctx->frame_number, buf_size);
@@ -630,15 +632,17 @@ retry:
goto retry;
}
-// for skipping the frame
-s->current_picture.f->pict_type = s->pict_type;
-s->current_picture.f->key_frame = s->pict_type == AV_PICTURE_TYPE_I;
+// for skipping the frame and keyframe markup
+pict_type = h->freeze_picture_release ? AV_PICTURE_TYPE_I : s->pict_type;
-if ((avctx->skip_frame >= AVDISCARD_NONREF && s->pict_type == AV_PICTURE_TYPE_B) ||
-(avctx->skip_frame >= AVDISCARD_NONKEY && s->pict_type !=
AV_PICTURE_TYPE_I) ||
+if ((avctx->skip_frame >= AVDISCARD_NONREF && pict_type ==
AV_PICTURE_TYPE_B) ||
+(avctx->skip_frame >= AVDISCARD_NONKEY && pict_type !=
AV_PICTURE_TYPE_I) ||
avctx->skip_frame >= AVDISCARD_ALL)
return get_consumed_bytes(s, buf_size);
+s->current_picture.f->pict_type = s->pict_type;
+s->current_picture.f->key_frame = s->pict_type == AV_PICTURE_TYPE_I;
+
if (ff_mpv_frame_start(s, avctx) < 0)
return -1;
@@ -660,6 +664,11 @@ retry:
if ((ret = av_frame_ref(pict, s->current_picture_ptr->f)) < 0)
return ret;
+
+// fix picture type and correctly mark keyframes
+pict->pict_type = pict_type;
+pict->key_frame = pict_type == AV_PICTURE_TYPE_I;
+
ff_print_debug_info(s, s->current_picture_ptr, pict);
*got_frame = 1;
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On 2019-10-26 21:15, Michael Niedermayer wrote: On Sat, Oct 26, 2019 at 02:05:27PM +0300, Andrey Semashev wrote: The decoder never marks pictures as I-frames, which results in no keyframe indication and incorrect frame skipping, in cases when keyframes should be decoded. This commit works around this decoder limitation and marks I-frames and keyframes based on "freeze picture release" bit in h261 picture header. This reflects h261enc behavior. --- libavcodec/h261.h| 1 + libavcodec/h261dec.c | 27 ++- 2 files changed, 19 insertions(+), 9 deletions(-) If the goal is correctly recognizing I frames then checking if all blocks are intra should be the most reliable In my case, the goal is to know when a keyframe is received, i.e. when the receiver can be reasonably sure it can start displaying/processing received frames. Including after some frames lost in transmission. According to H.261 spec, "freeze picture release" bit is what is intended to mark keyframes. To quote section 4.3.3: A signal from an encoder which has responded to a fast update request and allows a decoder to exit from its freeze picture mode and display decoded pictures in the normal manner. I'll reiterate that h261enc marks keyframes with this bit. that wont work for skiping though as it requires decoding Right. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On Sat, Oct 26, 2019 at 02:05:27PM +0300, Andrey Semashev wrote: > The decoder never marks pictures as I-frames, which results in no > keyframe indication and incorrect frame skipping, in cases when > keyframes should be decoded. > > This commit works around this decoder limitation and marks I-frames > and keyframes based on "freeze picture release" bit in h261 picture > header. This reflects h261enc behavior. > --- > libavcodec/h261.h| 1 + > libavcodec/h261dec.c | 27 ++- > 2 files changed, 19 insertions(+), 9 deletions(-) If the goal is correctly recognizing I frames then checking if all blocks are intra should be the most reliable that wont work for skiping though as it requires decoding [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
On 2019-10-26 15:49, Carl Eugen Hoyos wrote:
Am Sa., 26. Okt. 2019 um 13:12 Uhr schrieb Andrey Semashev
:
The decoder never marks pictures as I-frames, which results in no
keyframe indication and incorrect frame skipping, in cases when
keyframes should be decoded.
This commit works around this decoder limitation and marks I-frames
and keyframes based on "freeze picture release" bit in h261 picture
header. This reflects h261enc behavior.
diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c
index 14a874c45d..3b1711a21d 100644
--- a/libavcodec/h261dec.c
+++ b/libavcodec/h261dec.c
@@ -502,9 +502,9 @@ static int h261_decode_picture_header(H261Context *h)
s->avctx->framerate = (AVRational) { 3, 1001 };
/* PTYPE starts here */
-skip_bits1(>gb); /* split screen off */
-skip_bits1(>gb); /* camera off */
-skip_bits1(>gb); /* freeze picture release off */
+skip_bits1(>gb); /* split screen indicator */
+skip_bits1(>gb); /* document camera indicator */
+h->freeze_picture_release = get_bits1(>gb); /* freeze picture release */
format = get_bits1(>gb);
@@ -532,7 +532,8 @@ static int h261_decode_picture_header(H261Context *h)
/* H.261 has no I-frames, but if we pass AV_PICTURE_TYPE_I for the first
* frame, the codec crashes if it does not contain all I-blocks
- * (e.g. when a packet is lost). */
+ * (e.g. when a packet is lost). We will fix the picture type in the
+ * output frame based on h->freeze_picture_release later. */
s->pict_type = AV_PICTURE_TYPE_P;
Why can't you use freeze_picture_release here?
The comment says the decoder may crash on some input. I don't know if
this is true and I don't have any test files not produced by ffmpeg
itself to test.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
Am Sa., 26. Okt. 2019 um 13:12 Uhr schrieb Andrey Semashev
:
>
> The decoder never marks pictures as I-frames, which results in no
> keyframe indication and incorrect frame skipping, in cases when
> keyframes should be decoded.
>
> This commit works around this decoder limitation and marks I-frames
> and keyframes based on "freeze picture release" bit in h261 picture
> header. This reflects h261enc behavior.
> diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c
> index 14a874c45d..3b1711a21d 100644
> --- a/libavcodec/h261dec.c
> +++ b/libavcodec/h261dec.c
> @@ -502,9 +502,9 @@ static int h261_decode_picture_header(H261Context *h)
> s->avctx->framerate = (AVRational) { 3, 1001 };
>
> /* PTYPE starts here */
> -skip_bits1(>gb); /* split screen off */
> -skip_bits1(>gb); /* camera off */
> -skip_bits1(>gb); /* freeze picture release off */
> +skip_bits1(>gb); /* split screen indicator */
> +skip_bits1(>gb); /* document camera indicator */
> +h->freeze_picture_release = get_bits1(>gb); /* freeze picture release
> */
>
> format = get_bits1(>gb);
>
> @@ -532,7 +532,8 @@ static int h261_decode_picture_header(H261Context *h)
>
> /* H.261 has no I-frames, but if we pass AV_PICTURE_TYPE_I for the first
> * frame, the codec crashes if it does not contain all I-blocks
> - * (e.g. when a packet is lost). */
> + * (e.g. when a packet is lost). We will fix the picture type in the
> + * output frame based on h->freeze_picture_release later. */
> s->pict_type = AV_PICTURE_TYPE_P;
Why can't you use freeze_picture_release here?
Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] libavcodec/h261dec: Fix keyframe markup and frame skipping.
The decoder never marks pictures as I-frames, which results in no
keyframe indication and incorrect frame skipping, in cases when
keyframes should be decoded.
This commit works around this decoder limitation and marks I-frames
and keyframes based on "freeze picture release" bit in h261 picture
header. This reflects h261enc behavior.
---
libavcodec/h261.h| 1 +
libavcodec/h261dec.c | 27 ++-
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/libavcodec/h261.h b/libavcodec/h261.h
index 399a404b2b..6662d38d6d 100644
--- a/libavcodec/h261.h
+++ b/libavcodec/h261.h
@@ -37,6 +37,7 @@
typedef struct H261Context {
MpegEncContext s;
+int freeze_picture_release; // 1 if freeze picture release bit is set in
the picture header
int current_mba;
int mba_diff;
int mtype;
diff --git a/libavcodec/h261dec.c b/libavcodec/h261dec.c
index 14a874c45d..3b1711a21d 100644
--- a/libavcodec/h261dec.c
+++ b/libavcodec/h261dec.c
@@ -502,9 +502,9 @@ static int h261_decode_picture_header(H261Context *h)
s->avctx->framerate = (AVRational) { 3, 1001 };
/* PTYPE starts here */
-skip_bits1(>gb); /* split screen off */
-skip_bits1(>gb); /* camera off */
-skip_bits1(>gb); /* freeze picture release off */
+skip_bits1(>gb); /* split screen indicator */
+skip_bits1(>gb); /* document camera indicator */
+h->freeze_picture_release = get_bits1(>gb); /* freeze picture release */
format = get_bits1(>gb);
@@ -532,7 +532,8 @@ static int h261_decode_picture_header(H261Context *h)
/* H.261 has no I-frames, but if we pass AV_PICTURE_TYPE_I for the first
* frame, the codec crashes if it does not contain all I-blocks
- * (e.g. when a packet is lost). */
+ * (e.g. when a packet is lost). We will fix the picture type in the
+ * output frame based on h->freeze_picture_release later. */
s->pict_type = AV_PICTURE_TYPE_P;
h->gob_number = 0;
@@ -590,6 +591,7 @@ static int h261_decode_frame(AVCodecContext *avctx, void
*data,
H261Context *h = avctx->priv_data;
MpegEncContext *s = >s;
int ret;
+enum AVPictureType pict_type;
AVFrame *pict = data;
ff_dlog(avctx, "*frame %d size=%d\n", avctx->frame_number, buf_size);
@@ -630,15 +632,17 @@ retry:
goto retry;
}
-// for skipping the frame
-s->current_picture.f->pict_type = s->pict_type;
-s->current_picture.f->key_frame = s->pict_type == AV_PICTURE_TYPE_I;
+// for skipping the frame and keyframe markup
+pict_type = h->freeze_picture_release ? AV_PICTURE_TYPE_I : s->pict_type;
-if ((avctx->skip_frame >= AVDISCARD_NONREF && s->pict_type ==
AV_PICTURE_TYPE_B) ||
-(avctx->skip_frame >= AVDISCARD_NONKEY && s->pict_type !=
AV_PICTURE_TYPE_I) ||
+if ((avctx->skip_frame >= AVDISCARD_NONREF && pict_type ==
AV_PICTURE_TYPE_B) ||
+(avctx->skip_frame >= AVDISCARD_NONKEY && pict_type !=
AV_PICTURE_TYPE_I) ||
avctx->skip_frame >= AVDISCARD_ALL)
return get_consumed_bytes(s, buf_size);
+s->current_picture.f->pict_type = s->pict_type;
+s->current_picture.f->key_frame = s->pict_type == AV_PICTURE_TYPE_I;
+
if (ff_mpv_frame_start(s, avctx) < 0)
return -1;
@@ -660,6 +664,11 @@ retry:
if ((ret = av_frame_ref(pict, s->current_picture_ptr->f)) < 0)
return ret;
+
+// fix picture type and correctly mark keyframes
+pict->pict_type = pict_type;
+pict->key_frame = pict_type == AV_PICTURE_TYPE_I;
+
ff_print_debug_info(s, s->current_picture_ptr, pict);
*got_frame = 1;
--
2.20.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
