[FFmpeg-devel] [PATCH] avformat/mxf: Establish register of local tags

[Marton Balint]

On Wed, 27 Jan 2021, Tomas Härdin wrote:

> ons 2021-01-27 klockan 22:24 +0100 skrev Tomas Härdin:
>> ons 2021-01-27 klockan 21:38 +0100 skrev Marton Balint:
>>> On Wed, 27 Jan 2021, Tomas Härdin wrote:
>>>
>>>> Hi
>>>>
>>>> Ticket #9079 brought this about. This should prevent accidentally
>>>> adding local tags that are not registered in the primer. It also allows
>>>> us to omit tags that we know won't be used, in a manner that is more
>>>> elegant than the old code.
>>>>
>>>> The actual meat of this patch is mxf_mark_tag_unused(),
>>>> mxf_write_primer_pack(), mxf_write_local_tag() and
>>>> ff_mxf_lookup_local_tag()
>>>
>>> IMHO you should not move the local tags to mxf.c, because only encoding
>>> uses them.
>>>
>>> The only exception where sharing made sense is
>>> ff_mxf_mastering_display_local_tags, but that is super ugly that you
>>> now lookup them in mxfdec.c based on local tags we assign them for
>>> encoding. Not to mention the linear search you use for each lookup...
>>
>> We could sort them and use a binary search, but I wanted some feedback
>> on this idea before going further. There's not terribly many of them
>>
>> I'd like to avoid having the full ULs twice in the code. The only way I
>> can see how to do that is with #defines
>>
>>> So I suggest you simply duplicate the 4 UL-s to the single local tags
>>> array you make and keep them in mxfenc.c, that way you also don't have to
>>> specify the array size manually...
>>
>> That might conflict with Andreas' deduplication efforts. But yeah, the
>> thought did occur to me
>
> Here's an updated patch. Feedback welcome.

Thanks, I like this version much more. One comment is that I'd put an 
assert right into mxf_lookup_local_tag instead of returning NULL if a tag 
is not found, this way you can remove NULL-check asserts from individual 
places where mxf_lookup_local_tag is called. Otherwise seems all fine.

Regards,
Marton