[FFmpeg-devel] [PATCH 2/2] avcodec/videotoolbox: add hevc support
Aman Gupta
ffmpeg at tmm1.net
Wed Sep 27 05:49:34 EEST 2017
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.
>
> > +
> > + /*
> > + * unsigned int(2) general_profile_space;
> > + * unsigned int(1) general_tier_flag;
> > + * unsigned int(5) general_profile_idc;
> > + */
> > + AV_W8(p + 1, ptlc.profile_space << 6 |
> > + ptlc.tier_flag << 5 |
> > + ptlc.profile_idc);
> > +
> > + /* unsigned int(32) general_profile_compatibility_flags; */
> > + memcpy(p + 2, ptlc.profile_compatibility_flag, 4);
> > +
> > + /* unsigned int(48) general_constraint_indicator_flags; */
> > + AV_W8(p + 6, ptlc.progressive_source_flag << 7 |
> > + ptlc.interlaced_source_flag << 6 |
> > + ptlc.non_packed_constraint_flag << 5 |
> > + ptlc.frame_only_constraint_flag << 4);
> > + AV_W8(p + 7, 0);
> > + AV_W8(p + 8, 0);
> > + AV_W8(p + 9, 0);
> > + AV_W8(p + 10, 0);
> > + AV_W8(p + 11, 0);
> > +
> > + /* unsigned int(8) general_level_idc; */
> > + AV_W8(p + 12, ptlc.level_idc);
> > +
> > + /*
> > + * bit(4) reserved = ‘1111’b;
> > + * unsigned int(12) min_spatial_segmentation_idc;
> > + */
> > + AV_W8(p + 13, 0xf0 | (vui.min_spatial_segmentation_idc >> 4));
> > + AV_W8(p + 14, vui.min_spatial_segmentation_idc & 0xff);
> > +
> > + /*
> > + * bit(6) reserved = ‘111111’b;
> > + * unsigned int(2) parallelismType;
> > + */
> > + if (!vui.min_spatial_segmentation_idc)
> > + parallelismType = 0;
> > + else if (pps->entropy_coding_sync_enabled_flag &&
> pps->tiles_enabled_flag)
> > + parallelismType = 0;
> > + else if (pps->entropy_coding_sync_enabled_flag)
> > + parallelismType = 3;
> > + else if (pps->tiles_enabled_flag)
> > + parallelismType = 2;
> > + else
> > + parallelismType = 1;
> > + AV_W8(p + 15, 0xfc | parallelismType);
> > +
> > + /*
> > + * bit(6) reserved = ‘111111’b;
> > + * unsigned int(2) chromaFormat;
> > + */
> > + AV_W8(p + 16, sps->chroma_format_idc | 0xfc);
> > +
> > + /*
> > + * bit(5) reserved = ‘11111’b;
> > + * unsigned int(3) bitDepthLumaMinus8;
> > + */
> > + AV_W8(p + 17, (sps->bit_depth - 8) | 0xfc);
> > +
> > + /*
> > + * bit(5) reserved = ‘11111’b;
> > + * unsigned int(3) bitDepthChromaMinus8;
> > + */
> > + AV_W8(p + 18, (sps->bit_depth_chroma - 8) | 0xfc);
> > +
> > + /* bit(16) avgFrameRate; */
> > + AV_W8(p + 19, 0);
> > + AV_W8(p + 20, 0);
>
This could be AV_WB16() instead for instance.
> > +
> > + /*
> > + * bit(2) constantFrameRate;
> > + * bit(3) numTemporalLayers;
> > + * bit(1) temporalIdNested;
> > + * unsigned int(2) lengthSizeMinusOne;
> > + */
> > + AV_W8(p + 21, 0 << 6 |
> > + sps->max_sub_layers << 3 |
> > + sps->temporal_id_nesting_flag << 2 |
> > + 3);
> > +
> > + /* unsigned int(8) numOfArrays; */
> > + AV_W8(p + 22, 3);
> > +
> > + p += 23;
> > + /* vps */
> > + /*
> > + * bit(1) array_completeness;
> > + * unsigned int(1) reserved = 0;
> > + * unsigned int(6) NAL_unit_type;
> > + */
> > + AV_W8(p, 1 << 7 |
> > + HEVC_NAL_VPS & 0x3f);
> > + /* unsigned int(16) numNalus; */
> > + AV_W8(p + 1, 0);
> > + AV_W8(p + 2, 1);
>
Same here, with AV_WB16().
> > + /* unsigned int(16) nalUnitLength; */
> > + AV_W8(p + 3, vps->data_size >> 8);
> > + AV_W8(p + 4, vps->data_size & 0xffff);
>
etc, etc.
> > + /* bit(8*nalUnitLength) nalUnit; */
> > + memcpy(p + 5, vps->data, vps->data_size);
> > + p += 5 + vps->data_size;
> > +
> > + /* sps */
> > + AV_W8(p, 1 << 7 |
> > + HEVC_NAL_SPS & 0x3f);
> > + AV_W8(p + 1, 0);
> > + AV_W8(p + 2, 1);
> > + AV_W8(p + 3, sps->data_size >> 8);
> > + AV_W8(p + 4, sps->data_size & 0xffff);
> > + memcpy(p + 5, sps->data, sps->data_size);
> > + p += 5 + sps->data_size;
> > +
> > + /* pps */
> > + AV_W8(p, 1 << 7 |
> > + HEVC_NAL_PPS & 0x3f);
> > + AV_W8(p + 1, num_pps >> 8);
> > + AV_W8(p + 2, num_pps & 0xffff);
> > + p += 3;
> > + for (i = 0; i < MAX_PPS_COUNT; i++) {
> > + if (h->ps.pps_list[i]) {
>
> I think this hints that there's no guarantee that the entire buffer of
> size vt_extradata_size will be filled with data.
> Keeping that in mind, you should use av_mallocz() for vt_extradata.
>
> Also, why did you use MAX_PPS_COUNT here but not to set vt_extradata_size?
>
Hmm, I used MAX_PPS_COUNT in both loops (they should be identical as I
copy/pasted), and the calculated size is exact based on the size of the *PS
data. This is verified with the assert() at the end of this function. The
approach was copied wholesale from avcc_create().
>
>
> > + const HEVCPPS *pps = (const HEVCPPS
> *)h->ps.pps_list[i]->data;
> > + AV_W8(p + 0, pps->data_size >> 8);
> > + AV_W8(p + 1, pps->data_size & 0xffff);
> > + memcpy(p + 2, pps->data, pps->data_size);
> > + p += 2 + pps->data_size;
> > + }
> > + }
> > +
> > + av_assert0(p - vt_extradata == vt_extradata_size);
>
> This and all the pointer handling you did above makes me think the best
> solution would be to use the bytestream2's PutByteContext API instead.
>
> > +
> > + data = CFDataCreate(kCFAllocatorDefault, vt_extradata,
> vt_extradata_size);
>
> As i mentioned above, i don't think the entire buffer is guaranteed to
> be always filled to the last byte. The bytestream2 API would let you
> keep track of how much you wrote to it and pass that to this function.
>
I will check out bytestream2 and see if it simplifies things. I'm using an
extra loop to pre-calculate the size of the hvcC that I might be able to
get rid of with the bytestream2 API.
One advantage of the current approach is that it mimics the code in
libavformat/hevc.c's hvcc_write(). This will make it easier to keep them in
sync in the future.
>
> > + av_free(vt_extradata);
> > + return data;
> > +}
> > +
> > int ff_videotoolbox_buffer_create(VTContext *vtctx, AVFrame *frame)
> > {
> > av_buffer_unref(&frame->buf[0]);
> > @@ -445,6 +614,18 @@ static int videotoolbox_h264_end_frame(AVCodecContext
> *avctx)
> > return videotoolbox_common_end_frame(avctx, frame);
> > }
> >
> > +static int videotoolbox_hevc_end_frame(AVCodecContext *avctx)
> > +{
> > + HEVCContext *h = avctx->priv_data;
> > + AVFrame *frame = h->ref->frame;
> > + VTContext *vtctx = avctx->internal->hwaccel_priv_data;
> > + int ret;
> > +
> > + ret = videotoolbox_common_end_frame(avctx, frame);
> > + vtctx->bitstream_size = 0;
> > + return ret;
> > +}
> > +
> > static int videotoolbox_mpeg_start_frame(AVCodecContext *avctx,
> > const uint8_t *buffer,
> > uint32_t size)
> > @@ -501,6 +682,11 @@ static CFDictionaryRef videotoolbox_decoder_config_create(CMVideoCodecType
> codec
> > if (data)
> > CFDictionarySetValue(avc_info, CFSTR("avcC"), data);
> > break;
> > + case kCMVideoCodecType_HEVC :
> > + data = ff_videotoolbox_hvcc_extradata_create(avctx);
> > + if (data)
> > + CFDictionarySetValue(avc_info, CFSTR("hvcC"), data);
> > + break;
> > default:
> > break;
> > }
> > @@ -600,6 +786,9 @@ static int videotoolbox_default_init(AVCodecContext
> *avctx)
> > case AV_CODEC_ID_H264 :
> > videotoolbox->cm_codec_type = kCMVideoCodecType_H264;
> > break;
> > + case AV_CODEC_ID_HEVC :
> > + videotoolbox->cm_codec_type = kCMVideoCodecType_HEVC;
> > + break;
> > case AV_CODEC_ID_MPEG1VIDEO :
> > videotoolbox->cm_codec_type = kCMVideoCodecType_MPEG1Video;
> > break;
> > @@ -782,6 +971,20 @@ AVHWAccel ff_h263_videotoolbox_hwaccel = {
> > .priv_data_size = sizeof(VTContext),
> > };
> >
> > +AVHWAccel ff_hevc_videotoolbox_hwaccel = {
> > + .name = "hevc_videotoolbox",
> > + .type = AVMEDIA_TYPE_VIDEO,
> > + .id = AV_CODEC_ID_HEVC,
> > + .pix_fmt = AV_PIX_FMT_VIDEOTOOLBOX,
> > + .alloc_frame = ff_videotoolbox_alloc_frame,
> > + .start_frame = ff_videotoolbox_h264_start_frame,
> > + .decode_slice = ff_videotoolbox_h264_decode_slice,
> > + .end_frame = videotoolbox_hevc_end_frame,
> > + .init = videotoolbox_common_init,
> > + .uninit = ff_videotoolbox_uninit,
> > + .priv_data_size = sizeof(VTContext),
> > +};
> > +
> > AVHWAccel ff_h264_videotoolbox_hwaccel = {
> > .name = "h264_videotoolbox",
> > .type = AVMEDIA_TYPE_VIDEO,
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list