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

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@googlemail.com>
---
 libavcodec/cbs_h2645.c | 59 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 9 deletions(-)

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@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

I actually never liked these magic defines myself. So I followed
your proposal and in doing so I realized that it doesn't really
solve the need for these defines: One still has to differentiate the
case where a external buffer exists and the case where no such buffer
exists (because one needs a special free function for the first case,
unless one wants to create unnecessary free functions for the second
case). The only advantage such a solution has is that it is easily
extendable to the case where a struct contains more than one external
buffer. And this is a case that simply doesn't exist at the moment, so I
didn't really care about it and instead kept it simple.*

The SIZE define could be replaced by adding explicit macro arguments
whether the size element value is in bits or in bytes.

Given that one needs to have a special free callback for structures with
external buffers, it makes sense to automatically generate them together
with the copy functions. And this is what I did. These functions really
belong together. (I do not consider replacing the free functions with
macros to be obscure per se, but the old variable names was definitely
obscuring.)

The macro is in cbs_internal so that it can be used e.g. for
MPEG2RawUserData. Of course, it could be transferred to cbs_h2645.c.

ff_cbs_make_unit_writable is still called so, although it only makes the
unit's content writable, because it takes a unit argument. I wanted to
write generic code to (optionally) make the data writable, too, but I
refrained from it after I realized that the very definition of writable
isn't clear for slice units at all: Both the unit and the unit's content
contain a data_ref that usually point to the same buffer, so that a
unit's data could be said to be writable if it owns all references to
its data. Therefore determining the writability of the data would be
codec-dependent, so I left this can of worms closed for now (and probably
forever).



*: For the record, this is the structure I ended up using in this
approach:
struct {
    size_t    data_offset;
    size_t    ref_offset;
    size_t    size_offset;
    SizeType  size_type;
    SizeScale size_scale;
    size_t    plus_size;
};
SizeType is the type of the element containing the size.


Andreas Rheinhardt (5):
  cbs: Add function to make content of a unit writable
  cbs: Add a macro to create functions for deep copying
  cbs_h2645: Implement copy-functions for parameter sets
  cbs_h2645: Implement functions to make a unit's content writable
  h264_redundant_pps: Make it reference-compatible

 libavcodec/cbs.c                    |  14 ++++
 libavcodec/cbs.h                    |   6 ++
 libavcodec/cbs_av1.c                |   1 +
 libavcodec/cbs_h2645.c              | 112 +++++++++++++++++++++-------
 libavcodec/cbs_internal.h           |  57 ++++++++++++++
 libavcodec/cbs_jpeg.c               |   1 +
 libavcodec/cbs_mpeg2.c              |   1 +
 libavcodec/cbs_vp9.c                |   1 +
 libavcodec/h264_redundant_pps_bsf.c |  15 +++-
 9 files changed, 176 insertions(+), 32 deletions(-)