[FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches

[wm4]

On Sun, 11 Oct 2015 21:55:12 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote:
> > On Sun, 11 Oct 2015 21:16:27 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > From: Michael Niedermayer <michael at niedermayer.cc>
> > > 
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/h264_slice.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> > > index cce97d9..daa3737 100644
> > > --- a/libavcodec/h264_slice.c
> > > +++ b/libavcodec/h264_slice.c
> > > @@ -985,6 +985,10 @@ static enum AVPixelFormat get_pixel_format(H264Context *h, int force_callback)
> > >      for (i=0; choices[i] != AV_PIX_FMT_NONE; i++)
> > >          if (non_j_pixfmt(choices[i]) == non_j_pixfmt(h->avctx->pix_fmt) && !force_callback)
> > >              return choices[i];
> > > +
> > > +    if (!force_callback)
> > > +        return AV_PIX_FMT_NONE;
> > > +
> > >      return ff_thread_get_format(h->avctx, choices);
> > >  }
> > >  
> > 
> > So if I can see this right, the whole purpose of this is to check
> > whether the h264 parameters map to a lavc pixfmt, and bail out or
> > reinitialize if it doesn't. Why can't this be delayed later? What
> > actually needs it sooner than the first "real" get_format? For dealing
> 
> > with paramater changes, why can't it check the raw parameters instead?
> 
> The raw parameters are checked as well but iam not sure these checks
> are complete, they are more complex.
> 

I've checked again. 3 parameters influence the pixfmt:
bit_depth_luma, chroma_format_idc, and colorspace. Maybe add
color_range because of the J formats.  The first two are already
checked (lazily, if the SPS changes). A colorspace change also triggers
reinit, although the check for it is buried somewhat deeper in the
code. (Also I see a potential bug there: are colorspace and other
fields not reset correctly if the SPS changes, and the new SPS doesn't
have these fields set anymore?) A changing color_range does not force
reinit; this must be fixed (although I'd prefer dropping the J hack
completely).

So do you agree that checks for colorspace and color_range should be
added, and that they should trigger reinit, and that this combined with
my patch would fix all the potential issues the patch could introduce?

Note that because of hwaccels, get_format should always be triggered
if the SPS changes in any way, because some hwaccels might rely on the
current SPS information.

I'm also questioning why there are so many "clever" fine grained reinit
checks. Wouldn't it be better to fully reinit every time there is a new
SPS, and the new SPS is different than the previous SPS on the byte
level? (Don't worry, I'm only speaking hypothetically.)