Re: [FFmpeg-devel] [PATCH v2 06/11] cbs: Add support functions for handling unit content references
Mark Thompson:
> Use the unit type table to determine what we need to do to clone the
> internals of the unit content when making copies for refcounting or
> writeability. (This will still fail for units with complex content
> if they do not have a defined clone function.)
>
> Setup and naming from a patch by Andreas Rheinhardt
> , but with the implementation
> changed to use the unit type information if possible rather than
> requiring a codec-specific function.
> ---
> libavcodec/cbs.c | 168 ++
> libavcodec/cbs.h | 23 ++
> libavcodec/cbs_internal.h | 1 +
> 3 files changed, 192 insertions(+)
>
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 1963d86133..9fc8e1eb47 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -816,3 +816,171 @@ int ff_cbs_alloc_unit_content2(CodedBitstreamContext
> *ctx,
>
> return 0;
> }
> +
> +static int cbs_clone_unit_content(AVBufferRef **clone_ref,
> + CodedBitstreamUnit *unit,
> + const CodedBitstreamUnitTypeDescriptor
> *desc)
> +{
> +uint8_t *src, *copy;
> +void **src_ptr, **copy_ptr;
+ 1 to James' remark about the types.
> +AVBufferRef **src_buf, **copy_buf;
> +int err, i;
> +
> +av_assert0(unit->type == desc->unit_type);
> +av_assert0(unit->content);
> +src = unit->content;
> +
> +copy = av_malloc(desc->content_size);
> +if (!copy) {
> +err = AVERROR(ENOMEM);
> +goto fail;
You need to initialize i before going to fail; or even better, simply
return AVERROR(ENOMEM);
> +}
> +memcpy(copy, src, desc->content_size);
> +
> +for (i = 0; i < desc->nb_ref_offsets; i++) {
> +src_ptr = (void**)(src + desc->ref_offsets[i]);
> +src_buf = (AVBufferRef**)(src_ptr + 1);
> +copy_ptr = (void**)(copy + desc->ref_offsets[i]);
> +copy_buf = (AVBufferRef**)(copy_ptr + 1);
> +
> +if (!*src_ptr) {
> +av_assert0(!*src_buf);
> +continue;
> +}
> +if (!*src_buf) {
> +// We can't handle a non-refcounted pointer here - we don't
> +// have enough information to handle whatever structure lies
> +// at the other end of it.
> +err = AVERROR(EINVAL);
> +goto fail;
> +}
> +
> +// src_ptr is required to point somewhere inside src_buf. If it
> +// doesn't, there is a bug somewhere.
> +av_assert0((uint8_t*)*src_ptr >= (*src_buf)->data &&
> + (uint8_t*)*src_ptr < (*src_buf)->data +
> (*src_buf)->size);
> +
Just for completeness: Pointer comparisons on pointers that do not
point inside the same object (or one member past the end of an array)
is undefined behaviour, so the above is not a 100% kosher way to check
for whether src_ptr points inside src_buf. But it will probably work
anyway, so I don't mind.
> +*copy_buf = av_buffer_ref(*src_buf);
> +if (!*copy_buf) {
> +err = AVERROR(ENOMEM);
> +goto fail;
> +}
> +
> +err = av_buffer_make_writable(copy_buf);
> +if (err < 0) {
> +av_buffer_unref(copy_buf);
> +goto fail;
> +}
> +
> +*copy_ptr = (*copy_buf)->data +
> +((uint8_t*)*src_ptr - (*src_buf)->data);
> +}
> +
> +*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;
You forgot to set err to AVERROR(ENOMEM).
> +
> +return 0;
> +
> +fail:
> +for (--i; i >= 0; i--)
> +av_buffer_unref((AVBufferRef**)(copy + desc->ref_offsets[i]));
> +av_freep();
av_free(copy) is enough.
> +*clone_ref = NULL;
> +return err;
> +}
> +
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +CodedBitstreamUnit *unit)
> +{
> +const CodedBitstreamUnitTypeDescriptor *desc;
> +AVBufferRef *ref;
> +int err;
> +
> +if (unit->content_ref)
> +return 0;
> +
> +desc = cbs_find_unit_type_desc(ctx, unit);
> +if (!desc)
> +return AVERROR(ENOSYS);
> +
> +switch (desc->content_type) {
> +case CBS_CONTENT_TYPE_POD:
> +ref = av_buffer_alloc(desc->content_size);
> +if (!ref)
> +return AVERROR(ENOMEM);
> +memcpy(ref->data, unit->content, desc->content_size);
> +err = 0;
> +break;
> +
> +case CBS_CONTENT_TYPE_INTERNAL_REFS:
> +err = cbs_clone_unit_content(, unit, desc);
> +break;
> +
> +case CBS_CONTENT_TYPE_COMPLEX:
> +if (!desc->content_clone)
> +return AVERROR_PATCHWELCOME;
> +err = desc->content_clone(, unit);
> +break;
> +
> +default:
Re: [FFmpeg-devel] [PATCH v2 06/11] cbs: Add support functions for handling unit content references
On 5/20/2019 8:02 PM, Mark Thompson wrote:
> Use the unit type table to determine what we need to do to clone the
> internals of the unit content when making copies for refcounting or
> writeability. (This will still fail for units with complex content
> if they do not have a defined clone function.)
>
> Setup and naming from a patch by Andreas Rheinhardt
> , but with the implementation
> changed to use the unit type information if possible rather than
> requiring a codec-specific function.
> ---
> libavcodec/cbs.c | 168 ++
> libavcodec/cbs.h | 23 ++
> libavcodec/cbs_internal.h | 1 +
> 3 files changed, 192 insertions(+)
>
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 1963d86133..9fc8e1eb47 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -816,3 +816,171 @@ int ff_cbs_alloc_unit_content2(CodedBitstreamContext
> *ctx,
>
> return 0;
> }
> +
> +static int cbs_clone_unit_content(AVBufferRef **clone_ref,
> + CodedBitstreamUnit *unit,
> + const CodedBitstreamUnitTypeDescriptor
> *desc)
> +{
> +uint8_t *src, *copy;
> +void **src_ptr, **copy_ptr;
Why use void if you're going to cast these to uint8_t everywhere?
> +AVBufferRef **src_buf, **copy_buf;
> +int err, i;
> +
> +av_assert0(unit->type == desc->unit_type);
> +av_assert0(unit->content);
> +src = unit->content;
> +
> +copy = av_malloc(desc->content_size);
> +if (!copy) {
> +err = AVERROR(ENOMEM);
> +goto fail;
> +}
> +memcpy(copy, src, desc->content_size);
> +
> +for (i = 0; i < desc->nb_ref_offsets; i++) {
> +src_ptr = (void**)(src + desc->ref_offsets[i]);
> +src_buf = (AVBufferRef**)(src_ptr + 1);
> +copy_ptr = (void**)(copy + desc->ref_offsets[i]);
> +copy_buf = (AVBufferRef**)(copy_ptr + 1);
> +
> +if (!*src_ptr) {
> +av_assert0(!*src_buf);
> +continue;
> +}
> +if (!*src_buf) {
> +// We can't handle a non-refcounted pointer here - we don't
> +// have enough information to handle whatever structure lies
> +// at the other end of it.
> +err = AVERROR(EINVAL);
> +goto fail;
> +}
> +
> +// src_ptr is required to point somewhere inside src_buf. If it
> +// doesn't, there is a bug somewhere.
> +av_assert0((uint8_t*)*src_ptr >= (*src_buf)->data &&
> + (uint8_t*)*src_ptr < (*src_buf)->data +
> (*src_buf)->size);
> +
> +*copy_buf = av_buffer_ref(*src_buf);
> +if (!*copy_buf) {
> +err = AVERROR(ENOMEM);
> +goto fail;
> +}
> +
> +err = av_buffer_make_writable(copy_buf);
> +if (err < 0) {
> +av_buffer_unref(copy_buf);
> +goto fail;
> +}
Maybe instead of buffer_ref + make_writable, just do a buffer_alloc +
memcpy. It should be faster and only requires one failure check.
> +
> +*copy_ptr = (*copy_buf)->data +
> +((uint8_t*)*src_ptr - (*src_buf)->data);
> +}
> +
> +*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 err;
> +}
> +
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +CodedBitstreamUnit *unit)
> +{
> +const CodedBitstreamUnitTypeDescriptor *desc;
> +AVBufferRef *ref;
> +int err;
> +
> +if (unit->content_ref)
> +return 0;
> +
> +desc = cbs_find_unit_type_desc(ctx, unit);
> +if (!desc)
> +return AVERROR(ENOSYS);
> +
> +switch (desc->content_type) {
> +case CBS_CONTENT_TYPE_POD:
> +ref = av_buffer_alloc(desc->content_size);
> +if (!ref)
> +return AVERROR(ENOMEM);
> +memcpy(ref->data, unit->content, desc->content_size);
> +err = 0;
> +break;
> +
> +case CBS_CONTENT_TYPE_INTERNAL_REFS:
> +err = cbs_clone_unit_content(, unit, desc);
> +break;
> +
> +case CBS_CONTENT_TYPE_COMPLEX:
> +if (!desc->content_clone)
> +return AVERROR_PATCHWELCOME;
> +err = desc->content_clone(, unit);
> +break;
> +
> +default:
> +av_assert0(0 && "Invalid content type.");
> +}
> +
> +if (err < 0)
> +return err;
> +
> +av_buffer_unref(>content_ref);
> +unit->content_ref = ref;
> +unit->content = ref->data;
> +return 0;
> +}
> +
> +int
[FFmpeg-devel] [PATCH v2 06/11] cbs: Add support functions for handling unit content references
Use the unit type table to determine what we need to do to clone the
internals of the unit content when making copies for refcounting or
writeability. (This will still fail for units with complex content
if they do not have a defined clone function.)
Setup and naming from a patch by Andreas Rheinhardt
, but with the implementation
changed to use the unit type information if possible rather than
requiring a codec-specific function.
---
libavcodec/cbs.c | 168 ++
libavcodec/cbs.h | 23 ++
libavcodec/cbs_internal.h | 1 +
3 files changed, 192 insertions(+)
diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
index 1963d86133..9fc8e1eb47 100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -816,3 +816,171 @@ int ff_cbs_alloc_unit_content2(CodedBitstreamContext *ctx,
return 0;
}
+
+static int cbs_clone_unit_content(AVBufferRef **clone_ref,
+ CodedBitstreamUnit *unit,
+ const CodedBitstreamUnitTypeDescriptor *desc)
+{
+uint8_t *src, *copy;
+void **src_ptr, **copy_ptr;
+AVBufferRef **src_buf, **copy_buf;
+int err, i;
+
+av_assert0(unit->type == desc->unit_type);
+av_assert0(unit->content);
+src = unit->content;
+
+copy = av_malloc(desc->content_size);
+if (!copy) {
+err = AVERROR(ENOMEM);
+goto fail;
+}
+memcpy(copy, src, desc->content_size);
+
+for (i = 0; i < desc->nb_ref_offsets; i++) {
+src_ptr = (void**)(src + desc->ref_offsets[i]);
+src_buf = (AVBufferRef**)(src_ptr + 1);
+copy_ptr = (void**)(copy + desc->ref_offsets[i]);
+copy_buf = (AVBufferRef**)(copy_ptr + 1);
+
+if (!*src_ptr) {
+av_assert0(!*src_buf);
+continue;
+}
+if (!*src_buf) {
+// We can't handle a non-refcounted pointer here - we don't
+// have enough information to handle whatever structure lies
+// at the other end of it.
+err = AVERROR(EINVAL);
+goto fail;
+}
+
+// src_ptr is required to point somewhere inside src_buf. If it
+// doesn't, there is a bug somewhere.
+av_assert0((uint8_t*)*src_ptr >= (*src_buf)->data &&
+ (uint8_t*)*src_ptr < (*src_buf)->data + (*src_buf)->size);
+
+*copy_buf = av_buffer_ref(*src_buf);
+if (!*copy_buf) {
+err = AVERROR(ENOMEM);
+goto fail;
+}
+
+err = av_buffer_make_writable(copy_buf);
+if (err < 0) {
+av_buffer_unref(copy_buf);
+goto fail;
+}
+
+*copy_ptr = (*copy_buf)->data +
+((uint8_t*)*src_ptr - (*src_buf)->data);
+}
+
+*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 err;
+}
+
+int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
+CodedBitstreamUnit *unit)
+{
+const CodedBitstreamUnitTypeDescriptor *desc;
+AVBufferRef *ref;
+int err;
+
+if (unit->content_ref)
+return 0;
+
+desc = cbs_find_unit_type_desc(ctx, unit);
+if (!desc)
+return AVERROR(ENOSYS);
+
+switch (desc->content_type) {
+case CBS_CONTENT_TYPE_POD:
+ref = av_buffer_alloc(desc->content_size);
+if (!ref)
+return AVERROR(ENOMEM);
+memcpy(ref->data, unit->content, desc->content_size);
+err = 0;
+break;
+
+case CBS_CONTENT_TYPE_INTERNAL_REFS:
+err = cbs_clone_unit_content(, unit, desc);
+break;
+
+case CBS_CONTENT_TYPE_COMPLEX:
+if (!desc->content_clone)
+return AVERROR_PATCHWELCOME;
+err = desc->content_clone(, unit);
+break;
+
+default:
+av_assert0(0 && "Invalid content type.");
+}
+
+if (err < 0)
+return err;
+
+av_buffer_unref(>content_ref);
+unit->content_ref = ref;
+unit->content = ref->data;
+return 0;
+}
+
+int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
+ CodedBitstreamUnit *unit)
+{
+const CodedBitstreamUnitTypeDescriptor *desc;
+AVBufferRef *ref;
+int err;
+
+// This can only be applied to refcounted units.
+err = ff_cbs_make_unit_refcounted(ctx, unit);
+if (err < 0)
+return err;
+av_assert0(unit->content && unit->content_ref);
+
+if (av_buffer_is_writable(unit->content_ref))
+return 0;
+
+desc = cbs_find_unit_type_desc(ctx, unit);
+if (!desc)
+return AVERROR(ENOSYS);
