tor 2021-01-28 klockan 08:56 +0100 skrev 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.
Maybe av_assert0(0 && "you forgot to add a local tag to the registry")? /Tomas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
