[FFmpeg-devel] [PATCH 2/4] avformat/ttmlenc: enable writing out additional header values
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Wed Mar 31 12:29:48 EEST 2021
Jan Ekström:
> On Tue, Mar 30, 2021 at 12:19 PM Andreas Rheinhardt
> <andreas.rheinhardt at outlook.com> wrote:
>>
>> Jan Ekström:
>>> From: Jan Ekström <jan.ekstrom at 24i.com>
>>>
>>> This way the encoder may pass on the following values to the muxer:
>>> 1) Additional root "tt" element attributes, such as the subtitle
>>> canvas reference size.
>>> 2) Anything before the body element of the document, such as regions
>>> in the head element, which can configure styles.
>>>
>>> Signed-off-by: Jan Ekström <jan.ekstrom at 24i.com>
>>> ---
>>> libavcodec/ttmlenc.h | 5 +++
>>> libavformat/ttmlenc.c | 78 ++++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 78 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavcodec/ttmlenc.h b/libavcodec/ttmlenc.h
>>> index c1dd5ec990..c3bb11478d 100644
>>> --- a/libavcodec/ttmlenc.h
>>> +++ b/libavcodec/ttmlenc.h
>>> @@ -25,4 +25,9 @@
>>> #define TTMLENC_EXTRADATA_SIGNATURE "lavc-ttmlenc"
>>> #define TTMLENC_EXTRADATA_SIGNATURE_SIZE (sizeof(TTMLENC_EXTRADATA_SIGNATURE) - 1)
>>>
>>> +static const char ttml_default_namespacing[] =
>>> +" xmlns=\"http://www.w3.org/ns/ttml\"\n"
>>> +" xmlns:ttm=\"http://www.w3.org/ns/ttml#metadata\"\n"
>>> +" xmlns:tts=\"http://www.w3.org/ns/ttml#styling\"\n";
>>> +
>>> #endif /* AVCODEC_TTMLENC_H */
>>> diff --git a/libavformat/ttmlenc.c b/libavformat/ttmlenc.c
>>> index 940f8bbd4e..5d9ad6b756 100644
>>> --- a/libavformat/ttmlenc.c
>>> +++ b/libavformat/ttmlenc.c
>>> @@ -37,18 +37,23 @@ enum TTMLPacketType {
>>> PACKET_TYPE_DOCUMENT,
>>> };
>>>
>>> +struct TTMLHeaderParameters {
>>> + char *tt_element_params;
>>> + char *pre_body_elements;
>>> +};
>>> +
>>> typedef struct TTMLMuxContext {
>>> enum TTMLPacketType input_type;
>>> unsigned int document_written;
>>> + struct TTMLHeaderParameters header_params;
>>> } TTMLMuxContext;
>>>
>>> static const char ttml_header_text[] =
>>> "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
>>> "<tt\n"
>>> -" xmlns=\"http://www.w3.org/ns/ttml\"\n"
>>> -" xmlns:ttm=\"http://www.w3.org/ns/ttml#metadata\"\n"
>>> -" xmlns:tts=\"http://www.w3.org/ns/ttml#styling\"\n"
>>> +"%s"
>>> " xml:lang=\"%s\">\n"
>>> +"%s"
>>> " <body>\n"
>>> " <div>\n";
>>>
>>> @@ -72,6 +77,47 @@ static void ttml_write_time(AVIOContext *pb, const char tag[],
>>> tag, hour, min, sec, millisec);
>>> }
>>>
>>> +static int ttml_set_header_values_from_extradata(
>>> + AVCodecParameters *par, struct TTMLHeaderParameters *header_params)
>>> +{
>>> + size_t additional_data_size =
>>> + par->extradata_size - TTMLENC_EXTRADATA_SIGNATURE_SIZE;
>>> +
>>
>> Missing check that extradata_size is >= TTMLENC_EXTRADATA_SIGNATURE_SIZE.
>
> This function is only called if `ttml_ctx->input_type ==
> PACKET_TYPE_PARAGRAPH`, which is only set if `extradata_size >=
> TTMLENC_EXTRADATA_SIGNATURE_SIZE` (and if the , thus I thought that
> checking it again would be unnecessary. Of course, if you think a
> check is still needed, the following `!additional_data_size` check
> `additional_data_size <= 0`.
>
Sorry for forgetting this; it was not immediate from the diff.
But notice that replacing the !additional_data_size check by
additional_data_size <= 0 is nonsense as additional_data_size has an
unsigned type.
>>
>>> + if (!additional_data_size) {
>>> + header_params->tt_element_params =
>>> + av_strndup(ttml_default_namespacing,
>>> + sizeof(ttml_default_namespacing) - 1);
>>> + header_params->pre_body_elements = av_strndup("", 1);
>>
>> Why are you using av_strndup and not the ordinary av_strdup here?
>
> True, just got used to using the function with the max size defined.
>
>> Anyway, these allocations and the ones below are unnecessary as you only
>> need all of this when writing the header.
>
> Yes, I could either utilize a struct, or pass a pointer to
> values/struct. Just tried to be careful and not to re-utilize buffers
> too easily from the extradata as-is.
And why are you trying not to re-utilize buffers from extradata?
>
>>
>>> +
>>> + if (!header_params->tt_element_params ||
>>> + !header_params->pre_body_elements)
>>> + return AVERROR(ENOMEM);
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + {
>>> + char *value =
>>> + (char *)par->extradata + TTMLENC_EXTRADATA_SIGNATURE_SIZE;
>>> + size_t value_size = av_strnlen(value, additional_data_size);
>>> +
>>> + if (!(header_params->tt_element_params = av_strndup(value, value_size)))
>>> + return AVERROR(ENOMEM);
>>> +
>>> + additional_data_size -= value_size + 1;
>>> + value += value_size + 1;
>>> + if (additional_data_size <= 0)
>>> + return AVERROR_INVALIDDATA;
>>> +
>>> + value_size = av_strnlen(value, additional_data_size);
>>> +
>>> + if (!(header_params->pre_body_elements = av_strndup(value, value_size)))
>>> + return AVERROR(ENOMEM);
>>> +
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> static int ttml_write_header(AVFormatContext *ctx)
>>> {
>>> TTMLMuxContext *ttml_ctx = ctx->priv_data;
>>> @@ -103,8 +149,21 @@ static int ttml_write_header(AVFormatContext *ctx)
>>>
>>> avpriv_set_pts_info(st, 64, 1, 1000);
>>>
>>> - if (ttml_ctx->input_type == PACKET_TYPE_PARAGRAPH)
>>> - avio_printf(pb, ttml_header_text, printed_lang);
>>> + if (ttml_ctx->input_type == PACKET_TYPE_PARAGRAPH) {
>>> + int ret = ttml_set_header_values_from_extradata(
>>> + st->codecpar, &ttml_ctx->header_params);
>>> + if (ret < 0) {
>>> + av_log(ctx, AV_LOG_ERROR,
>>> + "Failed to parse TTML header values from extradata: "
>>> + "%s!\n", av_err2str(ret));
>>> + return ret;
>>> + }
>>> +
>>> + avio_printf(pb, ttml_header_text,
>>> + ttml_ctx->header_params.tt_element_params,
>>> + printed_lang,
>>> + ttml_ctx->header_params.pre_body_elements);
>>> + }
>>> }
>>>
>>> return 0;
>>> @@ -159,6 +218,14 @@ static int ttml_write_trailer(AVFormatContext *ctx)
>>> return 0;
>>> }
>>>
>>> +static void ttml_free(AVFormatContext *ctx)
>>> +{
>>> + TTMLMuxContext *ttml_ctx = ctx->priv_data;
>>> +
>>> + av_freep(&(ttml_ctx->header_params.tt_element_params));
>>> + av_freep(&(ttml_ctx->header_params.pre_body_elements));
>>
>> Unnecessary parentheses (on top of these allocations being unnecessary).
>>
>>> +}
>>> +
>>> AVOutputFormat ff_ttml_muxer = {
>>> .name = "ttml",
>>> .long_name = NULL_IF_CONFIG_SMALL("TTML subtitle"),
>>> @@ -171,4 +238,5 @@ AVOutputFormat ff_ttml_muxer = {
>>> .write_header = ttml_write_header,
>>> .write_packet = ttml_write_packet,
>>> .write_trailer = ttml_write_trailer,
>>> + .deinit = ttml_free,
>>> };
>>>
>>
More information about the ffmpeg-devel
mailing list