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

[Michael Niedermayer]

On Sun, Oct 11, 2015 at 10:43:04PM +0200, wm4 wrote:
> 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.)

i suspect that would break some file somewhere somehow

but iam happy with anything that works

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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151011/8a31a9d9/attachment.sig>