| Message ID | 20191026110527.29893-1-andrey.semashev@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am Sa., 26. Okt. 2019 um 13:12 Uhr schrieb Andrey Semashev <andrey.semashev@gmail.com>: > > 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) { 30000, 1001 }; > > /* PTYPE starts here */ > - skip_bits1(&s->gb); /* split screen off */ > - skip_bits1(&s->gb); /* camera off */ > - skip_bits1(&s->gb); /* freeze picture release off */ > + skip_bits1(&s->gb); /* split screen indicator */ > + skip_bits1(&s->gb); /* document camera indicator */ > + h->freeze_picture_release = get_bits1(&s->gb); /* freeze picture release */ > > format = get_bits1(&s->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
On 2019-10-26 15:49, Carl Eugen Hoyos wrote: > Am Sa., 26. Okt. 2019 um 13:12 Uhr schrieb Andrey Semashev > <andrey.semashev@gmail.com>: >> >> 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) { 30000, 1001 }; >> >> /* PTYPE starts here */ >> - skip_bits1(&s->gb); /* split screen off */ >> - skip_bits1(&s->gb); /* camera off */ >> - skip_bits1(&s->gb); /* freeze picture release off */ >> + skip_bits1(&s->gb); /* split screen indicator */ >> + skip_bits1(&s->gb); /* document camera indicator */ >> + h->freeze_picture_release = get_bits1(&s->gb); /* freeze picture release */ >> >> format = get_bits1(&s->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.
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 [...]
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: <quote> 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. </quote> I'll reiterate that h261enc marks keyframes with this bit. > that wont work for skiping though as it requires decoding Right.
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) { 30000, 1001 }; > > /* PTYPE starts here */ > - skip_bits1(&s->gb); /* split screen off */ > - skip_bits1(&s->gb); /* camera off */ > - skip_bits1(&s->gb); /* freeze picture release off */ > + skip_bits1(&s->gb); /* split screen indicator */ > + skip_bits1(&s->gb); /* document camera indicator */ > + h->freeze_picture_release = get_bits1(&s->gb); /* freeze picture release */ > > format = get_bits1(&s->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 = &h->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; >
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 [...]
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.
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) { 30000, 1001 }; /* PTYPE starts here */ - skip_bits1(&s->gb); /* split screen off */ - skip_bits1(&s->gb); /* camera off */ - skip_bits1(&s->gb); /* freeze picture release off */ + skip_bits1(&s->gb); /* split screen indicator */ + skip_bits1(&s->gb); /* document camera indicator */ + h->freeze_picture_release = get_bits1(&s->gb); /* freeze picture release */ format = get_bits1(&s->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 = &h->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;