On 12/08/2020 02:55, Andreas Rheinhardt wrote:
First of all: I only looked at the sei_user_data stuff yet.
Mark Thompson:
---
libavcodec/h264_metadata_bsf.c | 443 ++++++++++++++++++---------------
1 file changed, 242 insertions(+), 201 deletions(-)
diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index eb1503159b..1d00ccdfb8 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
...
@@ -78,6 +79,7 @@ typedef struct H264MetadataContext {
int crop_bottom;
const char *sei_user_data;
+ H264RawSEIPayload sei_user_data_payload;
int delete_filler;
...
+
+ // The current packet should be treated as a seek point for metadata
+ // insertion if any of:
+ // - It is the first packet in the stream.
+ // - It contains an SPS, indicating that a sequence might start here.
+ // - It is marked as containing a key frame.
+ seek_point = !ctx->done_first_au || has_sps ||
+ (pkt->flags & AV_PKT_FLAG_KEY);
+
+ if (ctx->sei_user_data && seek_point) {
+ err = ff_cbs_h264_add_sei_message(au, &ctx->sei_user_data_payload);
+ if (err < 0) {
+ av_log(bsf, AV_LOG_ERROR, "Failed to add user data SEI "
+ "message to access unit.\n");
+ goto fail;
+ }
Did you test this? With a sample with more than one seekpoint? It
shouldn't work. The reason is that the ownership of the SEI message
moves to the unit the SEI message will be attached to* (on success; on
failure, the SEI message will be freed for you) and (on success) it will
be unreferenced when the access unit gets reset. Notice that in this
case ctx->sei_user_data_payload won't be changed, but its pointers will
become dangling pointers and the second seek point will lead to a
use-after-free.
I see two ways to fix this:
My preferred solution is not to set the data_ref of the
H264RawSEIUserDataUnregistered at all. In this case one does not need to
allocate a buffer and copy the string at all -- the pointer can point
directly into the ctx->sei_user_data string. It should work; in this
case the point below about len + 1 not being in the range of int becomes
moot, of course. (Notice that due to using a PutBitContext which
currently uses an int anything longer than INT_MAX / 8 won't work anyway
-- but maybe one should nevertheless use a size_t for the loop that
writes the data in the user_data_(un)registered function?)
The second solution would be to keep a reference to the data_ref
containing the string from which to create references which are then
used in the SEI message; furthermore one would need to add code to free
this reference in the close function (yes, this is missing right now --
the buffer leaks if e.g. init fails after the buffer's allocation).
In any case I wonder whether the semantics of
ff_cbs_h264_add_sei_message() are good as is: They were designed for a
case in which the SEI message passed to it would go out of scope
immediately after the function call, so leaving it in a consistent state
was irrelevant. E.g. if one adds an SEI message with an internal ref,
there would now be two pointers to the corresponding AVBufferRef.
Furthermore freeing a user-data-unregistered SEI message currently only
unreferences the AVBufferRef; it does not reset the other fields (the
pointer might become dangling in this scenario). If the other fields
were reset, too, then one would need to store them separately outside of
the SEI message (maybe one should keep only the pointer to the data as
well as its length in the context and keep using SEI messages on the
stack?).
Moreover, I want to mention that allowing an internal buffer that is not
refcounted will cause slight complications if we ever wanted to copy the
containing unit or make it writable. But I am nevertheless in favour of
doing it here.
The simplicity of your preferred answer here is so much greater that I have to
agree.
Furthermore, we need better FATE-tests: This bsf seems to be only tested
by passthrough tests. No options are set. This bug here would probably
have been found by you earlier if there was a test with more than one
seekpoint.
+ }
+
+ if (ctx->delete_filler) {
+ for (i = au->nb_units - 1; i >= 0; i--) {
+ if (au->units[i].type == H264_NAL_FILLER_DATA) {
+ ff_cbs_delete_unit(au, i);
+ continue;
+ }
+
+ if (au->units[i].type == H264_NAL_SEI) {
+ // Filler SEI messages.
+ H264RawSEI *sei = au->units[i].content;
+
+ for (j = sei->payload_count - 1; j >= 0; j--) {
+ if (sei->payload[j].payload_type ==
+ H264_SEI_TYPE_FILLER_PAYLOAD)
+ ff_cbs_h264_delete_sei_message(au,
+ &au->units[i], j);
+ }
}
}
}
+ if (ctx->display_orientation != PASS) {
+ err = h264_metadata_handle_display_orientation(bsf, pkt, au,
+ seek_point);
+ if (err < 0)
+ goto fail;
+ }
+
err = ff_cbs_write_packet(ctx->output, pkt, au);
if (err < 0) {
av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
@@ -625,7 +616,57 @@ static int h264_metadata_init(AVBSFContext *bsf)
{
H264MetadataContext *ctx = bsf->priv_data;
CodedBitstreamFragment *au = &ctx->access_unit;
- int err, i;
+ int err, i, j;
+
+ if (ctx->sei_user_data) {
+ H264RawSEIUserDataUnregistered *udu;
+ int c, v;
These two could be moved to loop body scope.
Yep, moved.
+ size_t len;
The scope of this one should be the "Skip over the '+'." block below.
This variable disappeared anyway.
+
+ ctx->sei_user_data_payload = (H264RawSEIPayload) {
+ .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
+ };
ctx->sei_user_data_payload.payload_type =
H264_SEI_TYPE_USER_DATA_UNREGISTERED;
is enough as ctx is already zero initialized.
Sure, changed.
+ udu = &ctx->sei_user_data_payload.payload.user_data_unregistered;
+
+ // Parse UUID. It must be a hex string of length 32, possibly
+ // containing '-'s which we ignore.
+ for (i = j = 0; j < 32 && ctx->sei_user_data[i]; i++) {
This code has a potential for undefined behaviour/overflow (that already
existed before this patch): There is nothing that precludes the length
of ctx->sei_user_data from being huge (you just have to set
av_max_alloc() to a huge value). And if you have lots of '-', i might
overflow.
The easiest fix for this is to not use an index, but a pointer to be
incremented directly. This also means that you do not have to add a
second loop counter variable.
I decided to just ban having infinitely many -, because that's ridiculous. The
worst non-stupid (well, not-quite-as-stupid) case is a '-' between every digit,
so we bound i.
+ c = ctx->sei_user_data[i];
+ if (c == '-') {
+ continue;
+ } else if (av_isxdigit(c)) {
+ c = av_tolower(c);
+ v = (c <= '9' ? c - '0' : c - 'a' + 10);
+ } else {
+ break;
+ }
+ if (j & 1)
+ udu->uuid_iso_iec_11578[j / 2] |= v;
+ else
+ udu->uuid_iso_iec_11578[j / 2] = v << 4;
+ ++j;
+ }
+ if (j < 32 || ctx->sei_user_data[i] != '+') {
+ av_log(bsf, AV_LOG_ERROR, "Invalid user data: "
+ "must be of the form \"UUID+string\".\n");
+ return AVERROR(EINVAL);
+ } else {
+ // Skip over the '+'.
+ ++i;
+
+ // Length of the actual data to insert (could be zero).
+ len = strlen(ctx->sei_user_data + i);
+
+ udu->data_ref = av_buffer_alloc(len + 1);
len + 1 might not fit into an int.
+ if (!udu->data_ref)
+ return AVERROR(ENOMEM);
+
+ udu->data_length = len + 1;
Is it really to be expected that the terminating zero be written (yes, I
know, the old code did it, too; just asking)?
I followed the pattern of the x264 version SEI message, which includes the
terminator (since one of the original uses of this was forging that message).
+ udu->data = udu->data_ref->data;
+ memcpy(udu->data, ctx->sei_user_data + 1, len);
+ udu->data[len] = 0;
This is unnecessary: Just copy len + 1 elements (as the old code did).
Finally, I like that you move parsing of the sei_user_data string to
init; that's the proper place for it. But this should be done in a patch
of its own (it changes behaviour, something that I don't expect from a
pure refactoring patch).
Ok, split into two patches (following).
With a bit of luck git will produce a nice diff
from the other changes and not this mess here (but I don't really expect
it -- the diff is just to large).
Nope, as expected :)
Thanks,
- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
