Re: [FFmpeg-devel] [PATCH v2 6/7] avformat/matroskaenc: support writing Dynamic HDR10+ packet side data

From: Jerome Martinez <jerome@mediaarea.net>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH v2 6/7] avformat/matroskaenc: support writing Dynamic HDR10+ packet side data
Date: Fri, 24 Mar 2023 16:11:50 +0100
Message-ID: <816b6838-b1c3-9ecf-1c8a-2d5739ee681f@mediaarea.net> (raw)
In-Reply-To: <3c4ff584-e011-9fcf-f060-85cc8ded81ee@gmail.com>

On 24/03/2023 13:59, James Almer wrote:
> On 3/24/2023 8:58 AM, Anton Khirnov wrote:
>> Quoting James Almer (2023-03-24 12:50:40)
>>> On 3/24/2023 8:45 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2023-03-21 19:00:01)
>>>>> @@ -3095,6 +3153,12 @@ after_cues:
>>>>>                avio_seek(mkv->track.bc, 
>>>>> track->blockadditionmapping_offset, SEEK_SET);
>>>>>                   put_ebml_uint(mkv->track.bc, 
>>>>> MATROSKA_ID_TRACKMAXBLKADDID, track->max_blockaddid);
>>>>> +            if (track->max_blockaddid == 4) { // HDR10+
>>>>
>>>> Maybe this magic 4 deserves a name too?
>>>
>>> Is has no name. See
>>> https://www.matroska.org/technical/codec_specs.html#itu-t35-metadata
>>>
>>> It only defines a name for the BlockAddIDType, which i used, and
>>> constricts the BlockAddIDValue to be used for it to 4.
>>> It's not necessarily HDR10+ (i can remove that comment if it makes it
>>> look like it should be named), it's potentially any ITU-T T35 
>>> payload of
>>> which we only care about HDR10+, so i can't name it that. It's 
>>> simply 4.
>>
>> MATROSKA_BLOCKADDITIONAL_ITUT35_VAL then?
>
> 4 as BlockAddID is valid for any mapped BlockAddIDType value, not just 
> MATROSKA_BLOCK_ADD_ID_TYPE_ITU_T_T35.
> But ok, i'll add a #define for it. If we ever start writing other 
> types, like SMPTE ST 12-1 Timecode, we could just make sure to never 
> use 4 for it.

SMPTE ST 12-1 would have a BlockAddIDType of 121 [1] for differentiating 
timecode streams from ITU T.35, true, but using 4 as BlockAddID for 
something not ITU T.35 may create problems with some legacy decoders and 
would not be recommended.
So IMO flagging 4 as MATROSKA_BLOCKADDITIONAL_ITUT35_VAL would make 
sense there.
I suggested a patch to the Matroska spec for being explicit about this 
value 4 in [2].

A small comment about "// HDR10+", as it is not only for HDR10+ (ITU 
T.35 may contain lot of other things, depending on 
itu_t_t35_terminal_provider_code etc), IMO it should be commented as "// 
ITU T.35".

By the way, it is great to see this patch, it is very useful and would 
help in making a similar patch for SMPTE ST 12-1 timecode.

[1] 
https://github.com/ietf-wg-cellar/matroska-specification/blob/master/block_additional_mappings/smpte-st12-1-timecode.md
[2] https://github.com/ietf-wg-cellar/matroska-specification/pull/745
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".