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

[Michael Niedermayer]

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

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

[Hendrik Leppkes]

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

[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  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

[Hendrik Leppkes]

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

[wm4]

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

[wm4]

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

[wm4]

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

Re: [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 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

[wm4]

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