[FFmpeg-devel] [PATCH v6 21/22] h264_metadata_bsf: Refactor the filter function into smaller parts
Mark Thompson
sw at jkqxz.net
Thu Aug 13 02:16:23 EEST 2020
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
More information about the ffmpeg-devel
mailing list