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.rheinha...@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(©_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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
