Message ID | 20190123230114.1086-7-sw@jkqxz.net |
---|---|
State | New |
Headers | show |
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); > +
diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index deabf5dde5..cbf107528c 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -724,3 +724,95 @@ 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; + 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; + } + } + + *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; + + desc = cbs_find_unit_type_desc(ctx, unit); + if (!desc) + return AVERROR(ENOSYS); + + switch (desc->content_type) { + case CBS_CONTENT_TYPE_POD: + return av_buffer_make_writable(&unit->content_ref); + + case CBS_CONTENT_TYPE_INTERNAL_REFS: + err = cbs_clone_unit_content(&ref, unit, desc); + break; + + case CBS_CONTENT_TYPE_COMPLEX: + if (!desc->content_clone) + return AVERROR_PATCHWELCOME; + err = desc->content_clone(&ref, unit); + break; + + default: + av_assert0(0 && "Invalid content type."); + } + if (err < 0) + return err; + + av_buffer_unref(&unit->content_ref); + unit->content_ref = ref; + unit->content = ref->data; + return 0; +} diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index 1079f625bf..7e8f63139d 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -371,4 +371,17 @@ int ff_cbs_delete_unit(CodedBitstreamContext *ctx, int position); +/** + * 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 + * 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); + + #endif /* AVCODEC_CBS_H */ diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h index aefe1a81e7..c7cf7f2192 100644 --- a/libavcodec/cbs_internal.h +++ b/libavcodec/cbs_internal.h @@ -50,6 +50,7 @@ typedef struct CodedBitstreamUnitTypeDescriptor { size_t ref_offsets[CBS_MAX_REF_OFFSETS]; void (*content_free)(void *opaque, uint8_t *data); + int (*content_clone)(AVBufferRef **ref, CodedBitstreamUnit *unit); } CodedBitstreamUnitTypeDescriptor; typedef struct CodedBitstreamType {