Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, 11 Oct 2015 23:20:02 +0200 Hendrik Leppkes wrote: > On Sun, Oct 11, 2015 at 11:14 PM, wm4 wrote: > > On Sun, 11 Oct 2015 23:00:07 +0200 > > Hendrik Leppkes wrote: > > > >> On Sun, Oct 11, 2015 at 10:43 PM, wm4 wrote: > >> > On Sun, 11 Oct 2015 21:55:12 +0200 > >> > Michael Niedermayer wrote: > >> > > >> >> On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > >> >> > On Sun, 11 Oct 2015 21:16:27 +0200 > >> >> > Michael Niedermayer wrote: > >> >> > > >> >> > > From: Michael Niedermayer > >> >> > > > >> >> > > Signed-off-by: Michael Niedermayer > >> >> > > --- > >> >> > > 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? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, 11 Oct 2015 23:09:19 +0200 Michael Niedermayer wrote: > On Sun, Oct 11, 2015 at 10:43:04PM +0200, wm4 wrote: > > On Sun, 11 Oct 2015 21:55:12 +0200 > > Michael Niedermayer wrote: > > > > > On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > > > > On Sun, 11 Oct 2015 21:16:27 +0200 > > > > Michael Niedermayer wrote: > > > > > > > > > From: Michael Niedermayer > > > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > --- > > > > > 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 Then I'll post an alternative patch, which will separate pixfmt selection and calling get_format, or something. Still, my videotoolbox change kind of relies on a get_format invocation strictly on each SPS change, even though the API user can of course implement a change detection to avoid reinit selectively. (Since the API user itself has to call the decoder reinit function from within get_format.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, Oct 11, 2015 at 11:14 PM, wm4 wrote: > On Sun, 11 Oct 2015 23:00:07 +0200 > Hendrik Leppkes wrote: > >> On Sun, Oct 11, 2015 at 10:43 PM, wm4 wrote: >> > On Sun, 11 Oct 2015 21:55:12 +0200 >> > Michael Niedermayer wrote: >> > >> >> On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: >> >> > On Sun, 11 Oct 2015 21:16:27 +0200 >> >> > Michael Niedermayer wrote: >> >> > >> >> > > From: Michael Niedermayer >> >> > > >> >> > > Signed-off-by: Michael Niedermayer >> >> > > --- >> >> > > 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
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, 11 Oct 2015 23:00:07 +0200 Hendrik Leppkes wrote: > On Sun, Oct 11, 2015 at 10:43 PM, wm4 wrote: > > On Sun, 11 Oct 2015 21:55:12 +0200 > > Michael Niedermayer wrote: > > > >> On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > >> > On Sun, 11 Oct 2015 21:16:27 +0200 > >> > Michael Niedermayer wrote: > >> > > >> > > From: Michael Niedermayer > >> > > > >> > > Signed-off-by: Michael Niedermayer > >> > > --- > >> > > 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? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, Oct 11, 2015 at 10:43:04PM +0200, wm4 wrote: > On Sun, 11 Oct 2015 21:55:12 +0200 > Michael Niedermayer wrote: > > > On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > > > On Sun, 11 Oct 2015 21:16:27 +0200 > > > Michael Niedermayer wrote: > > > > > > > From: Michael Niedermayer > > > > > > > > Signed-off-by: Michael Niedermayer > > > > --- > > > > 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 signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, Oct 11, 2015 at 10:43 PM, wm4 wrote: > On Sun, 11 Oct 2015 21:55:12 +0200 > Michael Niedermayer wrote: > >> On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: >> > On Sun, 11 Oct 2015 21:16:27 +0200 >> > Michael Niedermayer wrote: >> > >> > > From: Michael Niedermayer >> > > >> > > Signed-off-by: Michael Niedermayer >> > > --- >> > > 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. > > 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.) > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, 11 Oct 2015 21:55:12 +0200 Michael Niedermayer wrote: > On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > > On Sun, 11 Oct 2015 21:16:27 +0200 > > Michael Niedermayer wrote: > > > > > From: Michael Niedermayer > > > > > > Signed-off-by: Michael Niedermayer > > > --- > > > 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.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, Oct 11, 2015 at 09:39:32PM +0200, wm4 wrote: > On Sun, 11 Oct 2015 21:16:27 +0200 > Michael Niedermayer wrote: > > > From: Michael Niedermayer > > > > Signed-off-by: Michael Niedermayer > > --- > > 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. > Doing it this way seems a bit convoluted. (I understand it now that I > thought about it, but normally I'd think it's VERY weird that it > somehow can go on without using the user-decided pixfmt, or that the > user-decided pixfmt sometimes doesn't seem to matter?) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
On Sun, 11 Oct 2015 21:16:27 +0200 Michael Niedermayer wrote: > From: Michael Niedermayer > > Signed-off-by: Michael Niedermayer > --- > 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? Doing it this way seems a bit convoluted. (I understand it now that I thought about it, but normally I'd think it's VERY weird that it somehow can go on without using the user-decided pixfmt, or that the user-decided pixfmt sometimes doesn't seem to matter?) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avcodec/h264_slice: Never call get_format() for just checking if the format matches
From: Michael Niedermayer Signed-off-by: Michael Niedermayer --- 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); } -- 1.7.9.5 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel