[FFmpeg-devel] [PATCH 2/6] cbs_h2645: Do a deep copy for parameter sets

[Mark Thompson]

On 09/11/18 05:31, Andreas Rheinhardt wrote:
> This commit solves dangling pointers problems when the content of a
> parameter set isn't refcounted in the beginning: Now a deep copy of the
> parameter sets is performed.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
>  libavcodec/cbs_h2645.c | 59 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 37b0207420..e73706f2e6 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -674,7 +674,26 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>      return 0;
>  }
>  
> -#define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \
> +
> +#define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element, buffer) \
> +static AVBufferRef* cbs_h26 ## h26n ## _copy_ ## ps_var(const H26 ## h26n ## Raw ## ps_name *source)\
> +{ \
> +    H26 ## h26n ## Raw ## ps_name *copy; \
> +    AVBufferRef *copy_ref; \
> +    copy = av_malloc(sizeof(*source)); \
> +    if (!copy) \
> +        return NULL; \
> +    memcpy(copy, source, sizeof(*source)); \
> +    copy_ref = av_buffer_create((uint8_t*)copy, sizeof(*source), \
> +                                FREE(h26n, ps_var), NULL, 0); \
> +    if (!copy_ref) { \
> +        av_free(copy); \
> +        return NULL; \
> +    } \
> +    cbs_h2645_copy_substructure(h26n, ps_name, ps_var, buffer) \
> +    return copy_ref; \
> +} \

I think the copy function should be split out from replace_ps, since you're calling it from other contexts in the following patches.

> + \
>  static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
>                                                    CodedBitstreamUnit *unit)  \
>  { \
> @@ -692,21 +711,43 @@ static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
>      if (unit->content_ref) \
>          priv->ps_var ## _ref[id] = av_buffer_ref(unit->content_ref); \
>      else \
> -        priv->ps_var ## _ref[id] = av_buffer_alloc(sizeof(*ps_var)); \
> +        priv->ps_var ## _ref[id] = cbs_h26 ## h26n ## _copy_ ## ps_var(ps_var); \
>      if (!priv->ps_var ## _ref[id]) \
>          return AVERROR(ENOMEM); \
>      priv->ps_var[id] = (H26 ## h26n ## Raw ## ps_name *)priv->ps_var ## _ref[id]->data; \
> -    if (!unit->content_ref) \
> -        memcpy(priv->ps_var[id], ps_var, sizeof(*ps_var)); \
>      return 0; \
>  }
>  
>  
> -cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id)
> -cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id)
> -cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id)
> -cbs_h2645_replace_ps(5, SPS, sps, sps_seq_parameter_set_id)
> -cbs_h2645_replace_ps(5, PPS, pps, pps_pic_parameter_set_id)
> +#define FREE(h26n, ps_var) NULL
> +#define cbs_h2645_copy_substructure(h26n, ps_name, ps_var, buffer)
> +cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id, )
> +#undef cbs_h2645_copy_substructure
> +#undef FREE
> +
> +#define FREE(h26n, ps_var) &cbs_h26 ## h26n ## _free_ ## ps_var
> +#define cbs_h2645_copy_substructure(h26n, ps_name, ps_var, buffer) \
> +    if (source->buffer) { \
> +        copy->buffer ## _ref = av_buffer_allocz(SIZE + AV_INPUT_BUFFER_PADDING_SIZE); \
> +        if (!copy->buffer) { \
> +            av_buffer_unref(&copy_ref); \
> +            return NULL; \
> +        } \
> +        copy->buffer = copy->buffer ## _ref->data; \
> +        memcpy(copy->buffer, source->buffer, SIZE); \
> +    }
> +
> +#define SIZE (copy->pic_size_in_map_units_minus1 + 1)
> +cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id, slice_group_id)
> +#undef SIZE
> +
> +#define SIZE ((copy->extension_data.bit_length + 7) / 8)
> +cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id, extension_data.data)
> +cbs_h2645_replace_ps(5, SPS, sps, sps_seq_parameter_set_id, extension_data.data)
> +cbs_h2645_replace_ps(5, PPS, pps, pps_pic_parameter_set_id, extension_data.data)
> +#undef SIZE
> +#undef FREE

Could we perhaps make a single "internal buffers" argument here rather than the additional magic defines (SIZE, FREE, copy_substructure)?

Something like:

#define cbs_h2645_copy_ps_with_buffers(h26n, ps_name, ps_var, id_element, internal_buffers) \
...
struct {
    uint8_t     *data;
    AVBufferRef *ref;
    size_t       size;
} bufs[] = { internal_buffers };
...
for (i = 0; i < FF_ARRAY_ELEMS(bufs); i++) {
    if (bufs[i].data) {
        ....
    }
}

(Though maybe it needs offsetof() against the structure rather than passing in names, not sure.)

> +
>  
>  static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
>                                    CodedBitstreamUnit *unit)
> 
Thanks,

- Mark