Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

On Mon, 25 Sep 2017 11:49:51 -0700
Aman Gupta  wrote:

> On Mon, Sep 25, 2017 at 3:06 AM, wm4  wrote:
> 
> > On Mon, 25 Sep 2017 09:02:36 +0200
> > Hendrik Leppkes  wrote:
> >  
> > > On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta  wrote:  
> > > >
> > > > How do the other hwaccels handle mid-stream SPS changes?
> > > >  
> > >
> > > Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS
> > > content for every frame, they don't keep a persistent state internally
> > > - that way the only "state" is the frame size and pixel format, and
> > > when those change get_format is called and the hwaccel re-initialized.  
> >
> > Maybe it would be better if VT detected SPS/PPS changes itself, and
> > reinitialized the VT session on demand when feeding slices. This way we
> > wouldn't have to mess with the normal h264 software decoder reinit
> > logic.
> >  
> 
> Agreed. I'm trying to figure out the most efficient way to detect when the
> SPS/PPS changes.
> 
> Previously I tried feeding in the SPS/PPS NALs from the decoder into the VT
> hwaccel (with a new `decode_params` callback), and using that to store a
> copy of the NAL in the hwaccel. I used a memcmp() against the previously
> stored value to detect when changes occurred and restarted the
> decompression session according. This approach worked well, but doesn't
> handle some streams which use multiple PPS.
> 
> The most fool-proof way would be to construct a new avcC every time and
> only restart the session when it changes. But that seems quite expensive to
> be doing all the time.
> 
> I'm also still not sure if the VT decoder needs to be restarted on SPS
> changes only, or PPS as well. Do new PPS usually accompany a new SPS? Is it
> also possible to have multiple SPS used at the same time? (The avcC
> construction code in videotoolbox.c assumes one SPS and one or more PPS).

I don't think it's too expensive. Keep in mind that we copy around the
actual slices 2 times as well, and the cost of rebuilding the avcc is
probably the smallest part, and the memcmp would barely matter.

Also I think what matters is whether VT sees the SPS/PPS slices at all.
Didn't you have success with feeding them as part of the sample buffer
(along with slice NALs)? I wonder if VT would be fine with the SPS and
PPS repeated for every slice.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[Aman Gupta]

On Mon, Sep 25, 2017 at 3:06 AM, wm4  wrote:

> On Mon, 25 Sep 2017 09:02:36 +0200
> Hendrik Leppkes  wrote:
>
> > On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta  wrote:
> > >
> > > How do the other hwaccels handle mid-stream SPS changes?
> > >
> >
> > Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS
> > content for every frame, they don't keep a persistent state internally
> > - that way the only "state" is the frame size and pixel format, and
> > when those change get_format is called and the hwaccel re-initialized.
>
> Maybe it would be better if VT detected SPS/PPS changes itself, and
> reinitialized the VT session on demand when feeding slices. This way we
> wouldn't have to mess with the normal h264 software decoder reinit
> logic.
>

Agreed. I'm trying to figure out the most efficient way to detect when the
SPS/PPS changes.

Previously I tried feeding in the SPS/PPS NALs from the decoder into the VT
hwaccel (with a new `decode_params` callback), and using that to store a
copy of the NAL in the hwaccel. I used a memcmp() against the previously
stored value to detect when changes occurred and restarted the
decompression session according. This approach worked well, but doesn't
handle some streams which use multiple PPS.

The most fool-proof way would be to construct a new avcC every time and
only restart the session when it changes. But that seems quite expensive to
be doing all the time.

I'm also still not sure if the VT decoder needs to be restarted on SPS
changes only, or PPS as well. Do new PPS usually accompany a new SPS? Is it
also possible to have multiple SPS used at the same time? (The avcC
construction code in videotoolbox.c assumes one SPS and one or more PPS).

Aman


> ___
> 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/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

On Mon, 25 Sep 2017 09:02:36 +0200
Hendrik Leppkes  wrote:

> On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta  wrote:
> >
> > How do the other hwaccels handle mid-stream SPS changes?
> >  
> 
> Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS
> content for every frame, they don't keep a persistent state internally
> - that way the only "state" is the frame size and pixel format, and
> when those change get_format is called and the hwaccel re-initialized.

Maybe it would be better if VT detected SPS/PPS changes itself, and
reinitialized the VT session on demand when feeding slices. This way we
wouldn't have to mess with the normal h264 software decoder reinit
logic.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[Hendrik Leppkes]

On Mon, Sep 25, 2017 at 3:31 AM, Aman Gupta  wrote:
>
> How do the other hwaccels handle mid-stream SPS changes?
>

Real HWAccels (ie. VAAPI, VDPAU or DXVA) communicate the SPS/PPS
content for every frame, they don't keep a persistent state internally
- that way the only "state" is the frame size and pixel format, and
when those change get_format is called and the hwaccel re-initialized.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[Aman Gupta]

On Thu, Oct 1, 2015 at 9:54 AM, wm4  wrote:

> On Thu, 1 Oct 2015 18:45:40 +0200
> Hendrik Leppkes  wrote:
>
> > On Thu, Oct 1, 2015 at 6:39 PM, wm4  wrote:
> > > On Thu, 1 Oct 2015 18:29:00 +0200
> > > Hendrik Leppkes  wrote:
> > >
> > >> On Thu, Oct 1, 2015 at 6:13 PM, wm4  wrote:
> > >> > This affects Annex B streams (such as demuxed from .ts and others).
> It
> > >> > also handles the format change in reinit-large_420_8-to-small_
> 420_8.h264
> > >> > correctly.
> > >> >
> > >> > Instead of passing through the extradata, create it on the fly it
> from
> > >> > the currently active SPS and PPS. Since reconstructing the PPS and
> SPS
> > >> > NALs would be very complicated and verbose, we use the NALs as they
> > >> > originally appeared in the bitstream.
> > >> >
> > >> > The code for writing the extradata is somewhat derived from
> > >> > libavformat/avc.c, but it's small and different enough that sharing
> it
> > >> > is not really worth it.
> > >> > ---
> > >> > Even though it requires changes in the general h264 decoder
> (previous
> > >> > patch), this solution is much cleaner and more robust than my patch
> > >> > from yesterday.
> > >> > ---
> > >> >  libavcodec/videotoolbox.c | 48 +-
> -
> > >> >  1 file changed, 30 insertions(+), 18 deletions(-)
> > >> >
> > >> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> > >> > index 9dec5fc..cc1e592 100644
> > >> > --- a/libavcodec/videotoolbox.c
> > >> > +++ b/libavcodec/videotoolbox.c
> > >> > @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext
> *avctx, AVFrame *frame)
> > >> >  return 0;
> > >> >  }
> > >> >
> > >> > +#define AV_W8(p, v) *(p) = (v)
> > >> > +
> > >> >  CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext
> *avctx)
> > >> >  {
> > >> > +H264Context *h = avctx->priv_data;
> > >> >  CFDataRef data = NULL;
> > >> > +uint8_t *p;
> > >> > +int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 +
> h->pps.data_size;
> > >> > +uint8_t *vt_extradata = av_malloc(vt_extradata_size);
> > >> > +if (!vt_extradata)
> > >> > +return NULL;
> > >> >
> > >> > -/* Each VCL NAL in the bitstream sent to the decoder
> > >> > - * is preceded by a 4 bytes length header.
> > >> > - * Change the avcC atom header if needed, to signal headers of
> 4 bytes. */
> > >> > -if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03)
> != 0x03) {
> > >> > -uint8_t *rw_extradata = av_memdup(avctx->extradata,
> avctx->extradata_size);
> > >> > -
> > >> > -if (!rw_extradata)
> > >> > -return NULL;
> > >> > -
> > >> > -rw_extradata[4] |= 0x03;
> > >> > -
> > >> > -data = CFDataCreate(kCFAllocatorDefault, rw_extradata,
> avctx->extradata_size);
> > >> > -
> > >> > -av_freep(_extradata);
> > >> > -} else {
> > >> > -data = CFDataCreate(kCFAllocatorDefault,
> avctx->extradata, avctx->extradata_size);
> > >> > -}
> > >> > -
> > >> > +p = vt_extradata;
> > >> > +
> > >> > +AV_W8(p + 0, 1); /* version */
> > >> > +AV_W8(p + 1, h->sps.data[0]); /* profile */
> > >> > +AV_W8(p + 2, h->sps.data[1]); /* profile compat */
> > >> > +AV_W8(p + 3, h->sps.data[2]); /* level */
> > >> > +AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal
> size length - 3 (11) */
> > >> > +AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number
> of sps (1) */
> > >> > +AV_WB16(p + 6, h->sps.data_size + 1);
> > >> > +AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
> > >> > +memcpy(p + 9, h->sps.data, h->sps.data_size);
> > >> > +p += 9 + h->sps.data_size;
> > >> > +AV_W8(p + 0, 1); /* number of pps */
> > >> > +AV_WB16(p + 1, h->pps.data_size + 1);
> > >> > +AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
> > >> > +memcpy(p + 4, h->pps.data, h->pps.data_size);
> > >> > +
> > >> > +p += 4 + h->pps.data_size;
> > >> > +av_assert0(p - vt_extradata == vt_extradata_size);
> > >> > +
> > >> > +data = CFDataCreate(kCFAllocatorDefault, vt_extradata,
> vt_extradata_size);
> > >> > +av_free(vt_extradata);
> > >> >  return data;
> > >> >  }
> > >> >
> > >>
> > >> This will still fail spectacularly with a SPS/PPS change mid-stream. I
> > >> don't suppose it somehow accepts the SPS/PPS data in-band as well?
> > >
> > > Well, it worked for the resolution change sample. It _should_ be using
> > > SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will
> > > reinitialize itself when they change (at least in most cases), and then
> > > it will also reinitialize this hwaccel, which means the code above is
> > > actually run in this situation. The h->sps and h->pps fields will be
> > > set to whatever is actually used by the decoder at this point. So I'm
> > > hoping that this 

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

On Thu,  1 Oct 2015 18:13:21 +0200
wm4  wrote:

> This affects Annex B streams (such as demuxed from .ts and others). It
> also handles the format change in reinit-large_420_8-to-small_420_8.h264
> correctly.
> 
> Instead of passing through the extradata, create it on the fly it from
> the currently active SPS and PPS. Since reconstructing the PPS and SPS
> NALs would be very complicated and verbose, we use the NALs as they
> originally appeared in the bitstream.
> 
> The code for writing the extradata is somewhat derived from
> libavformat/avc.c, but it's small and different enough that sharing it
> is not really worth it.
> ---
> Even though it requires changes in the general h264 decoder (previous
> patch), this solution is much cleaner and more robust than my patch
> from yesterday.
> ---
>  libavcodec/videotoolbox.c | 48 
> +--
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 

Both patches applied, with some requested amends (here, IRC) applied.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[Hendrik Leppkes]

On Thu, Oct 1, 2015 at 6:39 PM, wm4  wrote:
> On Thu, 1 Oct 2015 18:29:00 +0200
> Hendrik Leppkes  wrote:
>
>> On Thu, Oct 1, 2015 at 6:13 PM, wm4  wrote:
>> > This affects Annex B streams (such as demuxed from .ts and others). It
>> > also handles the format change in reinit-large_420_8-to-small_420_8.h264
>> > correctly.
>> >
>> > Instead of passing through the extradata, create it on the fly it from
>> > the currently active SPS and PPS. Since reconstructing the PPS and SPS
>> > NALs would be very complicated and verbose, we use the NALs as they
>> > originally appeared in the bitstream.
>> >
>> > The code for writing the extradata is somewhat derived from
>> > libavformat/avc.c, but it's small and different enough that sharing it
>> > is not really worth it.
>> > ---
>> > Even though it requires changes in the general h264 decoder (previous
>> > patch), this solution is much cleaner and more robust than my patch
>> > from yesterday.
>> > ---
>> >  libavcodec/videotoolbox.c | 48 
>> > +--
>> >  1 file changed, 30 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
>> > index 9dec5fc..cc1e592 100644
>> > --- a/libavcodec/videotoolbox.c
>> > +++ b/libavcodec/videotoolbox.c
>> > @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, 
>> > AVFrame *frame)
>> >  return 0;
>> >  }
>> >
>> > +#define AV_W8(p, v) *(p) = (v)
>> > +
>> >  CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx)
>> >  {
>> > +H264Context *h = avctx->priv_data;
>> >  CFDataRef data = NULL;
>> > +uint8_t *p;
>> > +int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + 
>> > h->pps.data_size;
>> > +uint8_t *vt_extradata = av_malloc(vt_extradata_size);
>> > +if (!vt_extradata)
>> > +return NULL;
>> >
>> > -/* Each VCL NAL in the bitstream sent to the decoder
>> > - * is preceded by a 4 bytes length header.
>> > - * Change the avcC atom header if needed, to signal headers of 4 
>> > bytes. */
>> > -if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 
>> > 0x03) {
>> > -uint8_t *rw_extradata = av_memdup(avctx->extradata, 
>> > avctx->extradata_size);
>> > -
>> > -if (!rw_extradata)
>> > -return NULL;
>> > -
>> > -rw_extradata[4] |= 0x03;
>> > -
>> > -data = CFDataCreate(kCFAllocatorDefault, rw_extradata, 
>> > avctx->extradata_size);
>> > -
>> > -av_freep(_extradata);
>> > -} else {
>> > -data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, 
>> > avctx->extradata_size);
>> > -}
>> > -
>> > +p = vt_extradata;
>> > +
>> > +AV_W8(p + 0, 1); /* version */
>> > +AV_W8(p + 1, h->sps.data[0]); /* profile */
>> > +AV_W8(p + 2, h->sps.data[1]); /* profile compat */
>> > +AV_W8(p + 3, h->sps.data[2]); /* level */
>> > +AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal size 
>> > length - 3 (11) */
>> > +AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps 
>> > (1) */
>> > +AV_WB16(p + 6, h->sps.data_size + 1);
>> > +AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
>> > +memcpy(p + 9, h->sps.data, h->sps.data_size);
>> > +p += 9 + h->sps.data_size;
>> > +AV_W8(p + 0, 1); /* number of pps */
>> > +AV_WB16(p + 1, h->pps.data_size + 1);
>> > +AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
>> > +memcpy(p + 4, h->pps.data, h->pps.data_size);
>> > +
>> > +p += 4 + h->pps.data_size;
>> > +av_assert0(p - vt_extradata == vt_extradata_size);
>> > +
>> > +data = CFDataCreate(kCFAllocatorDefault, vt_extradata, 
>> > vt_extradata_size);
>> > +av_free(vt_extradata);
>> >  return data;
>> >  }
>> >
>>
>> This will still fail spectacularly with a SPS/PPS change mid-stream. I
>> don't suppose it somehow accepts the SPS/PPS data in-band as well?
>
> Well, it worked for the resolution change sample. It _should_ be using
> SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will
> reinitialize itself when they change (at least in most cases), and then
> it will also reinitialize this hwaccel, which means the code above is
> actually run in this situation. The h->sps and h->pps fields will be
> set to whatever is actually used by the decoder at this point. So I'm
> hoping that this change is actually pretty correct.

Is init re-called on resolution change alone, everything else the same?
I guess it may actually work. Although there may still be other
SPS/PPS changes that don't trigger it, so it seems a bit fragile.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

On Thu, 1 Oct 2015 18:29:00 +0200
Hendrik Leppkes  wrote:

> On Thu, Oct 1, 2015 at 6:13 PM, wm4  wrote:
> > This affects Annex B streams (such as demuxed from .ts and others). It
> > also handles the format change in reinit-large_420_8-to-small_420_8.h264
> > correctly.
> >
> > Instead of passing through the extradata, create it on the fly it from
> > the currently active SPS and PPS. Since reconstructing the PPS and SPS
> > NALs would be very complicated and verbose, we use the NALs as they
> > originally appeared in the bitstream.
> >
> > The code for writing the extradata is somewhat derived from
> > libavformat/avc.c, but it's small and different enough that sharing it
> > is not really worth it.
> > ---
> > Even though it requires changes in the general h264 decoder (previous
> > patch), this solution is much cleaner and more robust than my patch
> > from yesterday.
> > ---
> >  libavcodec/videotoolbox.c | 48 
> > +--
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> > index 9dec5fc..cc1e592 100644
> > --- a/libavcodec/videotoolbox.c
> > +++ b/libavcodec/videotoolbox.c
> > @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, 
> > AVFrame *frame)
> >  return 0;
> >  }
> >
> > +#define AV_W8(p, v) *(p) = (v)
> > +
> >  CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx)
> >  {
> > +H264Context *h = avctx->priv_data;
> >  CFDataRef data = NULL;
> > +uint8_t *p;
> > +int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + 
> > h->pps.data_size;
> > +uint8_t *vt_extradata = av_malloc(vt_extradata_size);
> > +if (!vt_extradata)
> > +return NULL;
> >
> > -/* Each VCL NAL in the bitstream sent to the decoder
> > - * is preceded by a 4 bytes length header.
> > - * Change the avcC atom header if needed, to signal headers of 4 
> > bytes. */
> > -if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 
> > 0x03) {
> > -uint8_t *rw_extradata = av_memdup(avctx->extradata, 
> > avctx->extradata_size);
> > -
> > -if (!rw_extradata)
> > -return NULL;
> > -
> > -rw_extradata[4] |= 0x03;
> > -
> > -data = CFDataCreate(kCFAllocatorDefault, rw_extradata, 
> > avctx->extradata_size);
> > -
> > -av_freep(_extradata);
> > -} else {
> > -data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, 
> > avctx->extradata_size);
> > -}
> > -
> > +p = vt_extradata;
> > +
> > +AV_W8(p + 0, 1); /* version */
> > +AV_W8(p + 1, h->sps.data[0]); /* profile */
> > +AV_W8(p + 2, h->sps.data[1]); /* profile compat */
> > +AV_W8(p + 3, h->sps.data[2]); /* level */
> > +AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal size 
> > length - 3 (11) */
> > +AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps 
> > (1) */
> > +AV_WB16(p + 6, h->sps.data_size + 1);
> > +AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
> > +memcpy(p + 9, h->sps.data, h->sps.data_size);
> > +p += 9 + h->sps.data_size;
> > +AV_W8(p + 0, 1); /* number of pps */
> > +AV_WB16(p + 1, h->pps.data_size + 1);
> > +AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
> > +memcpy(p + 4, h->pps.data, h->pps.data_size);
> > +
> > +p += 4 + h->pps.data_size;
> > +av_assert0(p - vt_extradata == vt_extradata_size);
> > +
> > +data = CFDataCreate(kCFAllocatorDefault, vt_extradata, 
> > vt_extradata_size);
> > +av_free(vt_extradata);
> >  return data;
> >  }
> >
> 
> This will still fail spectacularly with a SPS/PPS change mid-stream. I
> don't suppose it somehow accepts the SPS/PPS data in-band as well?

Well, it worked for the resolution change sample. It _should_ be using
SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will
reinitialize itself when they change (at least in most cases), and then
it will also reinitialize this hwaccel, which means the code above is
actually run in this situation. The h->sps and h->pps fields will be
set to whatever is actually used by the decoder at this point. So I'm
hoping that this change is actually pretty correct.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

On Thu, 1 Oct 2015 18:45:40 +0200
Hendrik Leppkes  wrote:

> On Thu, Oct 1, 2015 at 6:39 PM, wm4  wrote:
> > On Thu, 1 Oct 2015 18:29:00 +0200
> > Hendrik Leppkes  wrote:
> >
> >> On Thu, Oct 1, 2015 at 6:13 PM, wm4  wrote:
> >> > This affects Annex B streams (such as demuxed from .ts and others). It
> >> > also handles the format change in reinit-large_420_8-to-small_420_8.h264
> >> > correctly.
> >> >
> >> > Instead of passing through the extradata, create it on the fly it from
> >> > the currently active SPS and PPS. Since reconstructing the PPS and SPS
> >> > NALs would be very complicated and verbose, we use the NALs as they
> >> > originally appeared in the bitstream.
> >> >
> >> > The code for writing the extradata is somewhat derived from
> >> > libavformat/avc.c, but it's small and different enough that sharing it
> >> > is not really worth it.
> >> > ---
> >> > Even though it requires changes in the general h264 decoder (previous
> >> > patch), this solution is much cleaner and more robust than my patch
> >> > from yesterday.
> >> > ---
> >> >  libavcodec/videotoolbox.c | 48 
> >> > +--
> >> >  1 file changed, 30 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> >> > index 9dec5fc..cc1e592 100644
> >> > --- a/libavcodec/videotoolbox.c
> >> > +++ b/libavcodec/videotoolbox.c
> >> > @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext 
> >> > *avctx, AVFrame *frame)
> >> >  return 0;
> >> >  }
> >> >
> >> > +#define AV_W8(p, v) *(p) = (v)
> >> > +
> >> >  CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx)
> >> >  {
> >> > +H264Context *h = avctx->priv_data;
> >> >  CFDataRef data = NULL;
> >> > +uint8_t *p;
> >> > +int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + 
> >> > h->pps.data_size;
> >> > +uint8_t *vt_extradata = av_malloc(vt_extradata_size);
> >> > +if (!vt_extradata)
> >> > +return NULL;
> >> >
> >> > -/* Each VCL NAL in the bitstream sent to the decoder
> >> > - * is preceded by a 4 bytes length header.
> >> > - * Change the avcC atom header if needed, to signal headers of 4 
> >> > bytes. */
> >> > -if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 
> >> > 0x03) {
> >> > -uint8_t *rw_extradata = av_memdup(avctx->extradata, 
> >> > avctx->extradata_size);
> >> > -
> >> > -if (!rw_extradata)
> >> > -return NULL;
> >> > -
> >> > -rw_extradata[4] |= 0x03;
> >> > -
> >> > -data = CFDataCreate(kCFAllocatorDefault, rw_extradata, 
> >> > avctx->extradata_size);
> >> > -
> >> > -av_freep(_extradata);
> >> > -} else {
> >> > -data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, 
> >> > avctx->extradata_size);
> >> > -}
> >> > -
> >> > +p = vt_extradata;
> >> > +
> >> > +AV_W8(p + 0, 1); /* version */
> >> > +AV_W8(p + 1, h->sps.data[0]); /* profile */
> >> > +AV_W8(p + 2, h->sps.data[1]); /* profile compat */
> >> > +AV_W8(p + 3, h->sps.data[2]); /* level */
> >> > +AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal size 
> >> > length - 3 (11) */
> >> > +AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps 
> >> > (1) */
> >> > +AV_WB16(p + 6, h->sps.data_size + 1);
> >> > +AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
> >> > +memcpy(p + 9, h->sps.data, h->sps.data_size);
> >> > +p += 9 + h->sps.data_size;
> >> > +AV_W8(p + 0, 1); /* number of pps */
> >> > +AV_WB16(p + 1, h->pps.data_size + 1);
> >> > +AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
> >> > +memcpy(p + 4, h->pps.data, h->pps.data_size);
> >> > +
> >> > +p += 4 + h->pps.data_size;
> >> > +av_assert0(p - vt_extradata == vt_extradata_size);
> >> > +
> >> > +data = CFDataCreate(kCFAllocatorDefault, vt_extradata, 
> >> > vt_extradata_size);
> >> > +av_free(vt_extradata);
> >> >  return data;
> >> >  }
> >> >
> >>
> >> This will still fail spectacularly with a SPS/PPS change mid-stream. I
> >> don't suppose it somehow accepts the SPS/PPS data in-band as well?
> >
> > Well, it worked for the resolution change sample. It _should_ be using
> > SPS/PPS NALs that occur mid-stream and in-band. The h264 decoder will
> > reinitialize itself when they change (at least in most cases), and then
> > it will also reinitialize this hwaccel, which means the code above is
> > actually run in this situation. The h->sps and h->pps fields will be
> > set to whatever is actually used by the decoder at this point. So I'm
> > hoping that this change is actually pretty correct.
> 
> Is init re-called on resolution change alone, everything else the same?
> I guess it may actually work. Although there may still be other
> SPS/PPS changes that don't trigger it, so it 

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[Hendrik Leppkes]

On Thu, Oct 1, 2015 at 6:13 PM, wm4  wrote:
> This affects Annex B streams (such as demuxed from .ts and others). It
> also handles the format change in reinit-large_420_8-to-small_420_8.h264
> correctly.
>
> Instead of passing through the extradata, create it on the fly it from
> the currently active SPS and PPS. Since reconstructing the PPS and SPS
> NALs would be very complicated and verbose, we use the NALs as they
> originally appeared in the bitstream.
>
> The code for writing the extradata is somewhat derived from
> libavformat/avc.c, but it's small and different enough that sharing it
> is not really worth it.
> ---
> Even though it requires changes in the general h264 decoder (previous
> patch), this solution is much cleaner and more robust than my patch
> from yesterday.
> ---
>  libavcodec/videotoolbox.c | 48 
> +--
>  1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index 9dec5fc..cc1e592 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, 
> AVFrame *frame)
>  return 0;
>  }
>
> +#define AV_W8(p, v) *(p) = (v)
> +
>  CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx)
>  {
> +H264Context *h = avctx->priv_data;
>  CFDataRef data = NULL;
> +uint8_t *p;
> +int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size;
> +uint8_t *vt_extradata = av_malloc(vt_extradata_size);
> +if (!vt_extradata)
> +return NULL;
>
> -/* Each VCL NAL in the bitstream sent to the decoder
> - * is preceded by a 4 bytes length header.
> - * Change the avcC atom header if needed, to signal headers of 4 bytes. 
> */
> -if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) {
> -uint8_t *rw_extradata = av_memdup(avctx->extradata, 
> avctx->extradata_size);
> -
> -if (!rw_extradata)
> -return NULL;
> -
> -rw_extradata[4] |= 0x03;
> -
> -data = CFDataCreate(kCFAllocatorDefault, rw_extradata, 
> avctx->extradata_size);
> -
> -av_freep(_extradata);
> -} else {
> -data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, 
> avctx->extradata_size);
> -}
> -
> +p = vt_extradata;
> +
> +AV_W8(p + 0, 1); /* version */
> +AV_W8(p + 1, h->sps.data[0]); /* profile */
> +AV_W8(p + 2, h->sps.data[1]); /* profile compat */
> +AV_W8(p + 3, h->sps.data[2]); /* level */
> +AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal size length 
> - 3 (11) */
> +AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps 
> (1) */
> +AV_WB16(p + 6, h->sps.data_size + 1);
> +AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
> +memcpy(p + 9, h->sps.data, h->sps.data_size);
> +p += 9 + h->sps.data_size;
> +AV_W8(p + 0, 1); /* number of pps */
> +AV_WB16(p + 1, h->pps.data_size + 1);
> +AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
> +memcpy(p + 4, h->pps.data, h->pps.data_size);
> +
> +p += 4 + h->pps.data_size;
> +av_assert0(p - vt_extradata == vt_extradata_size);
> +
> +data = CFDataCreate(kCFAllocatorDefault, vt_extradata, 
> vt_extradata_size);
> +av_free(vt_extradata);
>  return data;
>  }
>

This will still fail spectacularly with a SPS/PPS change mid-stream. I
don't suppose it somehow accepts the SPS/PPS data in-band as well?

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: fix decoding of some h264 bitstreams

[wm4]

This affects Annex B streams (such as demuxed from .ts and others). It
also handles the format change in reinit-large_420_8-to-small_420_8.h264
correctly.

Instead of passing through the extradata, create it on the fly it from
the currently active SPS and PPS. Since reconstructing the PPS and SPS
NALs would be very complicated and verbose, we use the NALs as they
originally appeared in the bitstream.

The code for writing the extradata is somewhat derived from
libavformat/avc.c, but it's small and different enough that sharing it
is not really worth it.
---
Even though it requires changes in the general h264 decoder (previous
patch), this solution is much cleaner and more robust than my patch
from yesterday.
---
 libavcodec/videotoolbox.c | 48 +--
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index 9dec5fc..cc1e592 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -77,28 +77,40 @@ int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, 
AVFrame *frame)
 return 0;
 }
 
+#define AV_W8(p, v) *(p) = (v)
+
 CFDataRef ff_videotoolbox_avcc_extradata_create(AVCodecContext *avctx)
 {
+H264Context *h = avctx->priv_data;
 CFDataRef data = NULL;
+uint8_t *p;
+int vt_extradata_size = 6 + 3 + h->sps.data_size + 4 + h->pps.data_size;
+uint8_t *vt_extradata = av_malloc(vt_extradata_size);
+if (!vt_extradata)
+return NULL;
 
-/* Each VCL NAL in the bitstream sent to the decoder
- * is preceded by a 4 bytes length header.
- * Change the avcC atom header if needed, to signal headers of 4 bytes. */
-if (avctx->extradata_size >= 4 && (avctx->extradata[4] & 0x03) != 0x03) {
-uint8_t *rw_extradata = av_memdup(avctx->extradata, 
avctx->extradata_size);
-
-if (!rw_extradata)
-return NULL;
-
-rw_extradata[4] |= 0x03;
-
-data = CFDataCreate(kCFAllocatorDefault, rw_extradata, 
avctx->extradata_size);
-
-av_freep(_extradata);
-} else {
-data = CFDataCreate(kCFAllocatorDefault, avctx->extradata, 
avctx->extradata_size);
-}
-
+p = vt_extradata;
+
+AV_W8(p + 0, 1); /* version */
+AV_W8(p + 1, h->sps.data[0]); /* profile */
+AV_W8(p + 2, h->sps.data[1]); /* profile compat */
+AV_W8(p + 3, h->sps.data[2]); /* level */
+AV_W8(p + 4, 0xff); /* 6 bits reserved (11) + 2 bits nal size length - 
3 (11) */
+AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps 
(1) */
+AV_WB16(p + 6, h->sps.data_size + 1);
+AV_W8(p + 8, NAL_SPS | (3 << 5)); // NAL unit header
+memcpy(p + 9, h->sps.data, h->sps.data_size);
+p += 9 + h->sps.data_size;
+AV_W8(p + 0, 1); /* number of pps */
+AV_WB16(p + 1, h->pps.data_size + 1);
+AV_W8(p + 3, NAL_PPS | (3 << 5)); // NAL unit header
+memcpy(p + 4, h->pps.data, h->pps.data_size);
+
+p += 4 + h->pps.data_size;
+av_assert0(p - vt_extradata == vt_extradata_size);
+
+data = CFDataCreate(kCFAllocatorDefault, vt_extradata, vt_extradata_size);
+av_free(vt_extradata);
 return data;
 }
 
-- 
2.5.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel