[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 <nfxjfg at googlemail.com> wrote:
> On Thu, 1 Oct 2015 18:29:00 +0200
> Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>
>> On Thu, Oct 1, 2015 at 6:13 PM, wm4 <nfxjfg at googlemail.com> 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(&rw_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 (111111) + 2 bits nal size length - 3 (11) */
>> > +    AV_W8(p + 5, 0xe1); /* 3 bits reserved (111) + 5 bits number of sps (00001) */
>> > +    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