From a macro point of view, I like your general approach using tables.
It really simplifies everything a lot.
(Rereading my old approach I don't know why I totally forgot that
there is a generic way to know the size of the internal buffers
(namely size of the AVBufferRefs). Not thinking of this, I opted for
the approach where the size is derived from other fields which made
everything very ungeneric and ugly. Sorry for wasting your time with
my earlier approach.)
Mark Thompson:
> +static int cbs_clone_unit_content(AVBufferRef **clone_ref,
> + CodedBitstreamUnit *unit,
> + const CodedBitstreamUnitTypeDescriptor
> *desc)
If this function were accessible from cbs_h2645.c, it could be used to
solve the dangling pointers problem in cbs_h26n_replace_xps by
performing a deep copy if the parameter sets are not refcounted (as I
did in "Implement copy-functions for parameter sets").
> +{
> + uint8_t *src, *copy;
> + AVBufferRef **src_buf, **copy_buf;
> + int err, i = 0;
> +
> + av_assert0(unit->type == desc->unit_type);
> + av_assert0(unit->content);
> + src = unit->content;
> +
> + copy = av_malloc(desc->content_size);
> + if (!copy)
> + goto fail;
> + memcpy(copy, src, desc->content_size);
> +
> + for (i = 0; i < desc->nb_ref_offsets; i++) {
> + src_buf = (AVBufferRef**)(src + desc->ref_offsets[i]);
> + copy_buf = (AVBufferRef**)(copy + desc->ref_offsets[i]);
> +
> + if (!*src_buf)
> + continue;
> +
> + *copy_buf = av_buffer_ref(*src_buf);
> + if (!*copy_buf)
> + goto fail;
> +
> + err = av_buffer_make_writable(copy_buf);
> + if (err < 0) {
> + av_buffer_unref(copy_buf);
> + goto fail;
> + }
You make the internal reference buffers writable, but you forgot that
access to the data held in these buffers is not performed via
content->buf_ref->data, but via content->(pointer to data). These
pointers need to be updated, too; and the offsets of the pointers will
have to be added to the CodedBitstreamUnitTypeDescriptor.
> + }
> +
> + *clone_ref = av_buffer_create(copy, desc->content_size,
> + desc->content_free ? desc->content_free :
> + cbs_default_free_unit_content,
> + (void*)desc, 0);
> + if (!*clone_ref)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + for (--i; i >= 0; i--)
> + av_buffer_unref((AVBufferRef**)(copy + desc->ref_offsets[i]));
> + av_freep(©);
> + *clone_ref = NULL;
> + return AVERROR(ENOMEM);
> +}
> +
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> + CodedBitstreamUnit *unit)
> +{
> + const CodedBitstreamUnitTypeDescriptor *desc;
> + AVBufferRef *ref;
> + int err;
> +
> + if (av_buffer_is_writable(unit->content_ref))
> + return 0;
This check has the implicit assumption that the content is refcounted;
is this intended?
> +/**
> + * Make the content of a unit writable so that internal fields can be
> + * modified.
> + *
> + * If there are no other references to the content of the unit, does
> + * nothing and returned success. Otherwise, it does a full clone of the
returns success.
> + * content (including any internal buffers) to make a new copy, and
> + * replaces the existing references inside the unit with that.
> + */
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> + CodedBitstreamUnit *unit);
> +
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
