[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 23:20:02 +0200
Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Sun, Oct 11, 2015 at 11:14 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Sun, 11 Oct 2015 23:00:07 +0200
> > Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >
> >> On Sun, Oct 11, 2015 at 10:43 PM, wm4 <nfxjfg at googlemail.com> 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?
> >>
> >>  Color Range and Color Space should not trigger a re-init, its
> >> pointless and may disrupt playback if a HWAccel re-inits.
> >> The in-memory representaiton, and as such the surface format, do not
> >> change when only these two change, so re-initing makes no sense to me.
> >> I have specifically hacked my local fork to avoid this because it can
> >> disrupt playback.
> >
> > What kind of stream did you deal with that changed color range/space so
> > often, and was it one of those single-sample-fixes?
> 
> Not "often", but I have seen many clips which start without this
> information and then suddenly they have it. If it needs to do a full
> re-init there, even only once, it can disrupt hwaccel decoding quite
> strongly and cause loss of frames or corrupted frames if your HWAccel
> re-inits.
> Even having that once in a clip is not acceptable if it can simply be avoided.

Do you mean the first N frames do not have it, and then the following
frames suddenly have it? Or is it a difference between the h264 parser
and decoder?