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

[Marton Balint]

On Thu, 28 Jan 2021, Tomas Härdin wrote:

> ons 2021-01-27 klockan 23:50 +0100 skrev 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.
>
> There's not really anything to av_assert0() on in
> mxf_lookup_local_tag(). Either way, I'm thinking replacing the return
> NULL with either
>
>    av_log(NULL, AV_LOG_PANIC, "Tried to use unregistered local tag
> 0x%04x\n", tag);
>    abort();
>
> or
>
>    av_assert0(0 && "Tried to use unregistered local tag");
>
> or maybe
>
>    av_log(NULL, AV_LOG_PANIC, "Tried to use unregistered local tag
> 0x%04x\n", tag);
>    av_assert0(0);
>
> to avoid explicitly calling abort()

I think we usually do av_assert0(0) in this case. I am not sure if the 
error message is particularly useful, afterall who sees it should be a 
programmer and file/line is printed by assert anyway, so a comment in the 
source code before the assert makes more sense to me.

Regards,
Marton