On Sun, Oct 11, 2015 at 11:14 PM, wm4 <nfx...@googlemail.com> wrote: > On Sun, 11 Oct 2015 23:00:07 +0200 > Hendrik Leppkes <h.lepp...@gmail.com> wrote: > >> On Sun, Oct 11, 2015 at 10:43 PM, wm4 <nfx...@googlemail.com> wrote: >> > On Sun, 11 Oct 2015 21:55:12 +0200 >> > Michael Niedermayer <mich...@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 <michae...@gmx.at> wrote: >> >> > >> >> > > From: Michael Niedermayer <mich...@niedermayer.cc> >> >> > > >> >> > > Signed-off-by: Michael Niedermayer <mich...@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. - Hendrik _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
