[FFmpeg-devel] [PATCH 4/6] avcodec/h264: keep track of which frames used gray references

[Michael Niedermayer]

On Sun, Nov 19, 2023 at 10:01:23PM -0500, Vittorio Giovara wrote:
> On Sun, Nov 19, 2023 at 7:11 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Tue, Nov 14, 2023 at 07:46:16PM +0100, Michael Niedermayer wrote:
> > > On Tue, Nov 14, 2023 at 06:32:19PM +0100, Hendrik Leppkes wrote:
> > > > On Tue, Nov 14, 2023 at 6:21 PM Michael Niedermayer
> > > > <michael at niedermayer.cc> wrote:
> > > > >
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/h264_picture.c |  1 +
> > > > >  libavcodec/h264_slice.c   | 19 ++++++++++++++++++-
> > > > >  libavcodec/h264dec.c      |  1 +
> > > > >  libavcodec/h264dec.h      |  4 ++++
> > > > >  4 files changed, 24 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c
> > > > > index 691c61eea23..3234141dbd6 100644
> > > > > --- a/libavcodec/h264_picture.c
> > > > > +++ b/libavcodec/h264_picture.c
> > > > > @@ -96,6 +96,7 @@ static void h264_copy_picture_params(H264Picture
> > *dst, const H264Picture *src)
> > > > >      dst->field_picture = src->field_picture;
> > > > >      dst->reference     = src->reference;
> > > > >      dst->recovered     = src->recovered;
> > > > > +    dst->gray          = src->gray;
> > > > >      dst->invalid_gap   = src->invalid_gap;
> > > > >      dst->sei_recovery_frame_cnt = src->sei_recovery_frame_cnt;
> > > > >      dst->mb_width      = src->mb_width;
> > > > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> > > > > index 4861a2cabba..2c4ab89dae1 100644
> > > > > --- a/libavcodec/h264_slice.c
> > > > > +++ b/libavcodec/h264_slice.c
> > > > > @@ -457,6 +457,7 @@ int ff_h264_update_thread_context(AVCodecContext
> > *dst,
> > > > >      h->poc.prev_frame_num        = h->poc.frame_num;
> > > > >
> > > > >      h->recovery_frame        = h1->recovery_frame;
> > > > > +    h->non_gray              = h1->non_gray;
> > > > >
> > > > >      return err;
> > > > >  }
> > > > > @@ -1544,11 +1545,14 @@ static int h264_field_start(H264Context *h,
> > const H264SliceContext *sl,
> > > > >                  if (ret < 0)
> > > > >                      return ret;
> > > > >                  h->short_ref[0]->poc = prev->poc + 2U;
> > > > > +                h->short_ref[0]->gray = prev->gray;
> > > > >                  ff_thread_report_progress(&h->short_ref[0]->tf,
> > INT_MAX, 0);
> > > > >                  if (h->short_ref[0]->field_picture)
> > > > >                      ff_thread_report_progress(&h->short_ref[0]->tf,
> > INT_MAX, 1);
> > > > > -            } else if (!h->frame_recovered && !h->avctx->hwaccel)
> > > > > +            } else if (!h->frame_recovered && !h->avctx->hwaccel) {
> > > > >                  color_frame(h->short_ref[0]->f, c);
> > > > > +                h->short_ref[0]->gray = 1;
> > > > > +            }
> > > >
> > > > While we can't color invalid frames for hwaccels easily, the flag
> > > > should perhaps still be applied, as they are equally invalid.
> > >
> > > ok
> >
> > will apply with this changed
> >
> > >
> > >
> > > > Thinking about this, maybe the name should be less the color we
> > > > happened to use for it, and more like "placeholder" or "invalid" or
> > > > anything like that?
> > >
> > > "invalid" is a quite generic term that covers more than this case.
> > > and iam not sure if "placeholder" is really good description of them.
> > > I picked "gray" because i had no better idea and they are gray
> >
> > if someone comes up with a better name, we can rename it
> > but gray is kind of fitting
> >
> 
> Sorry, what exactly are h264 gray frames?

Bascically when we have "missing" frames we fill them by copying
a frame close by or if we have none then we fill them with gray.
(this can occur due to lost frames or at the stream start before
 sei_recovery_frame_cnt)

These frames that we fill with gray are gray frames

Before this patchset we did not keep track of which frames where gray
so they could not be avoided as reference frames later. We only avoided
them during their creation.

After this patchset by default we avoid using gray frames as reference
which simply results in better looking frames.

Also i have a stream that is a classic IBBPBBP style stream but the
B frames use past B frames as references and accross intra frame so
that if gray frames are not avoided B frames will forever be corrupt
with gray blocks even after sei_recovery_frame_cnt.
Other H264 decoders do not show this behaviour that we have prior
to this patchset, they avoid these empty/gray frames.

Iam waiting for the owner of this stream to allow me to publish it
then ill make a fate test with it.

Basically this patchset makes things look better in every case where it
looks different than before that i found.

Also you can tune the options to get the old behavior or to drop
affected frames instead of adjusting references away from gray frames


> 
> Could you add some documentation to the new field in H264Picture too?

ill post a patch documenting that

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20231121/8d695330/attachment.sig>