[FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: add hevc support

[wm4]

On Tue, 26 Sep 2017 19:49:34 -0700
Aman Gupta <ffmpeg at tmm1.net> wrote:

> On Tue, Sep 26, 2017 at 7:20 PM, James Almer <jamrial at gmail.com> wrote:
> 
> > On 9/26/2017 10:08 PM, Aman Gupta wrote:  
> > > From: Aman Gupta <aman at tmm1.net>
> > >
> > > ---
> > >  configure                    |   2 +
> > >  libavcodec/allcodecs.c       |   1 +
> > >  libavcodec/hevc_refs.c       |   3 +
> > >  libavcodec/hevcdec.c         |  12 ++-
> > >  libavcodec/vda_vt_internal.h |   1 +
> > >  libavcodec/videotoolbox.c    | 203 ++++++++++++++++++++++++++++++  
> > +++++++++++++  
> > >  6 files changed, 221 insertions(+), 1 deletion(-)  
> >  
> > > +CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
> > > +{
> > > +    HEVCContext *h = avctx->priv_data;
> > > +    const HEVCVPS *vps = (const HEVCVPS *)h->ps.vps_list[0]->data;
> > > +    const HEVCSPS *sps = (const HEVCSPS *)h->ps.sps_list[0]->data;
> > > +    int i, num_pps = 0;
> > > +    const HEVCPPS *pps = h->ps.pps;  
> >  
> 
> One thing that surprised me.. when this is invoked, h->vps and h->sps are
> not yet set. Only h->pps has a value. I had to pull the vps and sps from
> the vps_list/sps_list directly.
> 
> 
> > > +    PTLCommon ptlc = vps->ptl.general_ptl;
> > > +    VUI vui = sps->vui;
> > > +    uint8_t parallelismType;
> > > +    CFDataRef data = NULL;
> > > +    uint8_t *p;
> > > +    int vt_extradata_size = 23 + 5 + vps->data_size + 5 +  
> > sps->data_size + 3;  
> > > +    uint8_t *vt_extradata;
> > > +
> > > +    for (i = 0; i < MAX_PPS_COUNT; i++) {
> > > +        if (h->ps.pps_list[i]) {
> > > +            const HEVCPPS *pps = (const HEVCPPS  
> > *)h->ps.pps_list[i]->data;  
> > > +            vt_extradata_size += 2 + pps->data_size;
> > > +            num_pps++;
> > > +        }
> > > +    }
> > > +
> > > +    vt_extradata = av_malloc(vt_extradata_size);
> > > +    if (!vt_extradata)
> > > +        return NULL;
> > > +    p = vt_extradata;
> > > +
> > > +    /* unsigned int(8) configurationVersion = 1; */
> > > +    AV_W8(p + 0, 1);  
> >
> > p is uint8_t*. Seems weird using AV_W8() for it.
> >
> > Yes, i know ff_videotoolbox_avcc_extradata_create() uses it, but there's
> > no reason to extend that behavior to new functions.
> >  
> 
> Are you recommending simple array access instead, i.e. `p[0] = 1`?
> 
> I just noticed the avcc creation is using AV_WB16() which would simplify
> some of my code.

AV_W8 doesn't actually exist - it's just a trivial macro defined in
videotoolbox.c. I added it because it looks more readable and symmetric,
especially mixed with AV_W16.

As for using bytestream writers etc. - I chose to manually write the
data, because this was the simplest at the time.