[FFmpeg-devel] [PATCH 2/2] avcodec/cbs_h2645: use AVBufferRef to store list of active parameter sets
James Almer
jamrial at gmail.com
Wed May 9 02:05:12 EEST 2018
On 5/8/2018 7:37 PM, Mark Thompson wrote:
> On 08/05/18 21:48, James Almer wrote:
>> This is more in line to how h264_ps handles it.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> Not really going to make much of a difference seeing parameter sets
>> rarely show up in-band even in raw streams, but it's one less memcpy
>> anyway.
>>
>> libavcodec/cbs_h264.h | 4 +--
>> libavcodec/cbs_h2645.c | 41 ++++++++++++++-------------
>> libavcodec/cbs_h264_syntax_template.c | 19 ++++++-------
>> libavcodec/cbs_h265.h | 6 ++--
>> libavcodec/cbs_h265_syntax_template.c | 21 +++++++-------
>> 5 files changed, 44 insertions(+), 47 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> index becea3adfe..239f3b6b90 100644
>> --- a/libavcodec/cbs_h264.h
>> +++ b/libavcodec/cbs_h264.h
>> @@ -420,8 +420,8 @@ typedef struct CodedBitstreamH264Context {
>>
>> // All currently available parameter sets. These are updated when
>> // any parameter set NAL unit is read/written with this context.
>> - H264RawSPS *sps[H264_MAX_SPS_COUNT];
>> - H264RawPPS *pps[H264_MAX_PPS_COUNT];
>> + AVBufferRef *sps[H264_MAX_SPS_COUNT];
>> + AVBufferRef *pps[H264_MAX_PPS_COUNT];
>
> Can we keep the current xps variables as they are and add a separate xps_ref? That doesn't significantly increase the complexity of the replacement functions, and would avoid the ugly casting in the read/write templated code (indeed, I think it removes all changes there entirely).
If you mean making it
AVBufferRef *sps_ref[H264_MAX_SPS_COUNT];
AVBufferRef *pps_ref[H264_MAX_PPS_COUNT];
H264RawSPS *sps[H264_MAX_SPS_COUNT];
H264RawPPS *pps[H264_MAX_PPS_COUNT];
Then sure, it indeed simplifies the patch a lot. Will send an updated
version in a moment.
>
>>
>> // The currently active parameter sets. These are updated when any
>> // NAL unit refers to the relevant parameter set. These pointers
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 580ca09f63..6c81a2e1b3 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -676,22 +676,23 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>>
>> #define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \
>> static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
>> - const H26 ## h26n ## Raw ## ps_name *ps_var) \
>> + CodedBitstreamUnit *unit) \
>> { \
>> CodedBitstreamH26 ## h26n ## Context *priv = ctx->priv_data; \
>> + H26 ## h26n ## Raw ## ps_name *ps_var = unit->content; \
>> unsigned int id = ps_var->id_element; \
>> if (id > FF_ARRAY_ELEMS(priv->ps_var)) { \
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid " #ps_name \
>> " id : %d.\n", id); \
>> return AVERROR_INVALIDDATA; \
>> } \
>> - if (priv->ps_var[id] == priv->active_ ## ps_var) \
>> + if (priv->ps_var[id] && \
>> + (H26 ## h26n ## Raw ## ps_name *) priv->ps_var[id]->data == priv->active_ ## ps_var) \
>> priv->active_ ## ps_var = NULL ; \
>> - av_freep(&priv->ps_var[id]); \
>> - priv->ps_var[id] = av_malloc(sizeof(*ps_var)); \
>> + av_buffer_unref(&priv->ps_var[id]); \
>> + priv->ps_var[id] = av_buffer_ref(unit->content_ref); \
>> if (!priv->ps_var[id]) \
>> return AVERROR(ENOMEM); \
>> - memcpy(priv->ps_var[id], ps_var, sizeof(*ps_var)); \
>> return 0; \
>> }
>>
>> @@ -725,7 +726,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h264_replace_sps(ctx, sps);
>> + err = cbs_h264_replace_sps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -759,7 +760,7 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h264_replace_pps(ctx, pps);
>> + err = cbs_h264_replace_pps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -872,7 +873,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_vps(ctx, vps);
>> + err = cbs_h265_replace_vps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -891,7 +892,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_sps(ctx, sps);
>> + err = cbs_h265_replace_sps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -911,7 +912,7 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_pps(ctx, pps);
>> + err = cbs_h265_replace_pps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1001,7 +1002,7 @@ static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h264_replace_sps(ctx, sps);
>> + err = cbs_h264_replace_sps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1025,7 +1026,7 @@ static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h264_replace_pps(ctx, pps);
>> + err = cbs_h264_replace_pps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1122,7 +1123,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_vps(ctx, vps);
>> + err = cbs_h265_replace_vps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1136,7 +1137,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_sps(ctx, sps);
>> + err = cbs_h265_replace_sps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1150,7 +1151,7 @@ static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
>> if (err < 0)
>> return err;
>>
>> - err = cbs_h265_replace_pps(ctx, pps);
>> + err = cbs_h265_replace_pps(ctx, unit);
>> if (err < 0)
>> return err;
>> }
>> @@ -1384,9 +1385,9 @@ static void cbs_h264_close(CodedBitstreamContext *ctx)
>> av_freep(&h264->common.write_buffer);
>>
>> for (i = 0; i < FF_ARRAY_ELEMS(h264->sps); i++)
>> - av_freep(&h264->sps[i]);
>> + av_buffer_unref(&h264->sps[i]);
>> for (i = 0; i < FF_ARRAY_ELEMS(h264->pps); i++)
>> - av_freep(&h264->pps[i]);
>> + av_buffer_unref(&h264->pps[i]);
>> }
>>
>> static void cbs_h265_close(CodedBitstreamContext *ctx)
>> @@ -1399,11 +1400,11 @@ static void cbs_h265_close(CodedBitstreamContext *ctx)
>> av_freep(&h265->common.write_buffer);
>>
>> for (i = 0; i < FF_ARRAY_ELEMS(h265->vps); i++)
>> - av_freep(&h265->vps[i]);
>> + av_buffer_unref(&h265->vps[i]);
>> for (i = 0; i < FF_ARRAY_ELEMS(h265->sps); i++)
>> - av_freep(&h265->sps[i]);
>> + av_buffer_unref(&h265->sps[i]);
>> for (i = 0; i < FF_ARRAY_ELEMS(h265->pps); i++)
>> - av_freep(&h265->pps[i]);
>> + av_buffer_unref(&h265->pps[i]);
>> }
>>
>> const CodedBitstreamType ff_cbs_type_h264 = {
>> diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
>> index c3327f6e90..6e4155baac 100644
>> --- a/libavcodec/cbs_h264_syntax_template.c
>> +++ b/libavcodec/cbs_h264_syntax_template.c
>> @@ -368,7 +368,7 @@ static int FUNC(pps)(CodedBitstreamContext *ctx, RWContext *rw,
>> ue(pic_parameter_set_id, 0, 255);
>> ue(seq_parameter_set_id, 0, 31);
>>
>> - sps = h264->sps[current->seq_parameter_set_id];
>> + sps = (const H264RawSPS *)h264->sps[current->seq_parameter_set_id]->data;
>> if (!sps) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "SPS id %d not available.\n",
>> current->seq_parameter_set_id);
>> @@ -471,13 +471,12 @@ static int FUNC(sei_buffering_period)(CodedBitstreamContext *ctx, RWContext *rw,
>>
>> ue(seq_parameter_set_id, 0, 31);
>>
>> - sps = h264->sps[current->seq_parameter_set_id];
>> - if (!sps) {
>> + if (!h264->sps[current->seq_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "SPS id %d not available.\n",
>> current->seq_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h264->active_sps = sps;
>> + h264->active_sps = sps = (const H264RawSPS *)h264->sps[current->seq_parameter_set_id]->data;
>>
>> if (sps->vui.nal_hrd_parameters_present_flag) {
>> for (i = 0; i <= sps->vui.nal_hrd_parameters.cpb_cnt_minus1; i++) {
>> @@ -578,7 +577,7 @@ static int FUNC(sei_pic_timing)(CodedBitstreamContext *ctx, RWContext *rw,
>> }
>> }
>> if (k >= 0)
>> - sps = h264->sps[k];
>> + sps = (const H264RawSPS *)h264->sps[k]->data;
>> }
>> if (!sps) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR,
>> @@ -1100,21 +1099,19 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>
>> ue(pic_parameter_set_id, 0, 255);
>>
>> - pps = h264->pps[current->pic_parameter_set_id];
>> - if (!pps) {
>> + if (!h264->pps[current->pic_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "PPS id %d not available.\n",
>> current->pic_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h264->active_pps = pps;
>> + h264->active_pps = pps = (const H264RawPPS *)h264->pps[current->pic_parameter_set_id]->data;
>>
>> - sps = h264->sps[pps->seq_parameter_set_id];
>> - if (!sps) {
>> + if (!h264->sps[pps->seq_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "SPS id %d not available.\n",
>> pps->seq_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h264->active_sps = sps;
>> + h264->active_sps = sps = (const H264RawSPS *)h264->sps[pps->seq_parameter_set_id]->data;
>>
>> if (sps->separate_colour_plane_flag)
>> u(2, colour_plane_id, 0, 2);
>> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h
>> index 1b357293ab..4eec99e9a8 100644
>> --- a/libavcodec/cbs_h265.h
>> +++ b/libavcodec/cbs_h265.h
>> @@ -522,9 +522,9 @@ typedef struct CodedBitstreamH265Context {
>>
>> // All currently available parameter sets. These are updated when
>> // any parameter set NAL unit is read/written with this context.
>> - H265RawVPS *vps[HEVC_MAX_VPS_COUNT];
>> - H265RawSPS *sps[HEVC_MAX_SPS_COUNT];
>> - H265RawPPS *pps[HEVC_MAX_PPS_COUNT];
>> + AVBufferRef *vps[HEVC_MAX_VPS_COUNT];
>> + AVBufferRef *sps[HEVC_MAX_SPS_COUNT];
>> + AVBufferRef *pps[HEVC_MAX_PPS_COUNT];
>>
>> // The currently active parameter sets. These are updated when any
>> // NAL unit refers to the relevant parameter set. These pointers
>> diff --git a/libavcodec/cbs_h265_syntax_template.c b/libavcodec/cbs_h265_syntax_template.c
>> index 9f13061f38..bc9e6ddf08 100644
>> --- a/libavcodec/cbs_h265_syntax_template.c
>> +++ b/libavcodec/cbs_h265_syntax_template.c
>> @@ -690,11 +690,12 @@ static int FUNC(sps)(CodedBitstreamContext *ctx, RWContext *rw,
>> CHECK(FUNC(nal_unit_header)(ctx, rw, ¤t->nal_unit_header, HEVC_NAL_SPS));
>>
>> u(4, sps_video_parameter_set_id, 0, 15);
>> - h265->active_vps = vps = h265->vps[current->sps_video_parameter_set_id];
>> + h265->active_vps = vps = NULL;
>>
>> u(3, sps_max_sub_layers_minus1, 0, HEVC_MAX_SUB_LAYERS - 1);
>> flag(sps_temporal_id_nesting_flag);
>> - if (vps) {
>> + if (h265->vps[current->sps_video_parameter_set_id]) {
>> + h265->active_vps = vps = (const H265RawVPS *)h265->vps[current->sps_video_parameter_set_id]->data;
>> if (vps->vps_max_sub_layers_minus1 > current->sps_max_sub_layers_minus1) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid stream: "
>> "sps_max_sub_layers_minus1 (%d) must be less than or equal to "
>> @@ -949,13 +950,13 @@ static int FUNC(pps)(CodedBitstreamContext *ctx, RWContext *rw,
>>
>> ue(pps_pic_parameter_set_id, 0, 63);
>> ue(pps_seq_parameter_set_id, 0, 15);
>> - sps = h265->sps[current->pps_seq_parameter_set_id];
>> - if (!sps) {
>> +
>> + if (!h265->sps[current->pps_seq_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "SPS id %d not available.\n",
>> current->pps_seq_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h265->active_sps = sps;
>> + h265->active_sps = sps = (const H265RawSPS *)h265->sps[current->pps_seq_parameter_set_id]->data;
>>
>> flag(dependent_slice_segments_enabled_flag);
>> flag(output_flag_present_flag);
>> @@ -1224,21 +1225,19 @@ static int FUNC(slice_segment_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>
>> ue(slice_pic_parameter_set_id, 0, 63);
>>
>> - pps = h265->pps[current->slice_pic_parameter_set_id];
>> - if (!pps) {
>> + if (!h265->pps[current->slice_pic_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "PPS id %d not available.\n",
>> current->slice_pic_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h265->active_pps = pps;
>> + h265->active_pps = pps = (const H265RawPPS *)h265->pps[current->slice_pic_parameter_set_id]->data;
>>
>> - sps = h265->sps[pps->pps_seq_parameter_set_id];
>> - if (!sps) {
>> + if (!h265->sps[pps->pps_seq_parameter_set_id]) {
>> av_log(ctx->log_ctx, AV_LOG_ERROR, "SPS id %d not available.\n",
>> pps->pps_seq_parameter_set_id);
>> return AVERROR_INVALIDDATA;
>> }
>> - h265->active_sps = sps;
>> + h265->active_sps = sps = (const H265RawSPS *)h265->sps[pps->pps_seq_parameter_set_id]->data;
>>
>> min_cb_log2_size_y = sps->log2_min_luma_coding_block_size_minus3 + 3;
>> ctb_log2_size_y = min_cb_log2_size_y + sps->log2_diff_max_min_luma_coding_block_size;
>>
>
> Good change. (I'm realising now that it fixes the possible dangling references held in pointers in the stored parameter sets too, which I hadn't thought about before.)
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list