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

[James Almer]

On 3/24/2023 12:11 PM, Jerome Martinez wrote:
> 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

Currently, the block_additional_mappings page only lists SMPTE ST 12-1 
timecode. For the rest (ITU-T T.35, Dovi stuff, Opaque data, etc) you 
need to stumble upon them in 
https://www.matroska.org/technical/codec_specs.html#block-addition-mappings, 
which is confusing.
Could these be added to the proper section too?

> [2] https://github.com/ietf-wg-cellar/matroska-specification/pull/745
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".