Re: [FFmpeg-devel] [PATCH] avcodec/itut35: always check the provider code and country code together - Maryla Ustarroz via ffmpeg-devel

From: Maryla Ustarroz via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>
To: Devin Heitmueller <devin.heitmueller@ltnglobal.com>
Cc: Maryla Ustarroz <maryla@google.com>,
	FFmpeg development discussions and patches
	<ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avcodec/itut35: always check the provider code and country code together
Date: Fri, 13 Jun 2025 12:57:55 +0200
Message-ID: <mailman.3174.1749812293.1384.ffmpeg-devel@ffmpeg.org> (raw)
In-Reply-To: <CAHGibzE1ZCuACYF-CkGSjEut+B-e50y=1GWtm7EfVDzb3tcxKA@mail.gmail.com>

[-- Attachment #1: Type: message/rfc822, Size: 5749 bytes --]

From: Maryla Ustarroz <maryla@google.com>
To: Devin Heitmueller <devin.heitmueller@ltnglobal.com>
Cc: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avcodec/itut35: always check the provider code and country code together
Date: Fri, 13 Jun 2025 12:57:55 +0200
Message-ID: <CA+yX6GEs_WYNv+nv5KnY4mzoqhToqmzdnHRpWkhKMXphbeH--g@mail.gmail.com>

On Thu, Jun 12, 2025 at 6:48 PM Devin Heitmueller
<devin.heitmueller@ltnglobal.com> wrote:
>
> On Thu, Jun 12, 2025 at 12:27 PM Maryla Ustarroz-Calonge via
> ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
> > ---------- Forwarded message ----------
> > From: Maryla Ustarroz-Calonge <maryla@google.com>
> > To: ffmpeg-devel@ffmpeg.org
> > Cc:
> > Bcc:
> > Date: Thu, 12 Jun 2025 18:27:43 +0200
> > Subject: [PATCH] avcodec/itut35: always check the provider code and country code together
> > From: Maryla <maryla@google.com>
> >
> > ITU-T T.35 provider codes are attributed by national bodies and it's
> > possible to have collisions across countries. This is why the country code
> > must always be checked as well.
> > In the code this could be done by having an outer switch on the country code,
> > then an inner switch on the provider code, but this would add an extra level of
> > indentation and is not necessary as long as the codes used don't collide.
> >
> > Rename some of the constants to match the corresponding organization.
> > Add a constant for AOM.
> > Write all constants with 4 hex digits to make it clear that they are 2-byte ids.
>
> While it's a bit more annoying, I think the checks should actually be
> inverted.  It should be looking at the country codes first, and then
> within those country codes it should look at the provider.

Thank you for the feedback.
I agree this would be cleaner on paper, but I just tried it and I
actually find it really hard to read and follow with all the nested
switches.
I think that simple ifs like this would be clearer:

if (country_code == ITU_T_T35_COUNTRY_CODE_US && provider_code ==
ITU_T_T35_PROVIDER_CODE_ATSC) {
   ...
} else if (country_code == ITU_T_T35_COUNTRY_CODE_US && provider_code
== ITU_T_T35_PROVIDER_CODE_SAMSUNG) {
   ...
} else if (...) {
etc.

This would also avoid adding an extra indentation level.
Of course, nested ifs are also possible.
Let me know what you think.

> Devin
>
> --
> Devin Heitmueller, Senior Software Engineer
> LTN Global Communications
> o: +1 (301) 363-1001
> w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".