[FFmpeg-devel] [PATCH 1/7] avcodec/pcm: Remove always-false check

[Ramiro Polla]

On Sat, Mar 15, 2025 at 5:56 AM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
> Ramiro Polla:
> > On 3/13/25 06:49, Andreas Rheinhardt wrote:
> >> Patches attached.
>
> > [PATCH 3/7] avcodec/pcm: Remove duplication from FFCodec define macros
> >
> >> diff --git a/libavcodec/pcm.c b/libavcodec/pcm.c
> >> index 620acf0f46..6c1feecca3 100644
> >> --- a/libavcodec/pcm.c
> >> +++ b/libavcodec/pcm.c
> > [...]
> >> +//       AV_CODEC_ID_*       pcm_* name
> >> +//                 AV_SAMPLE_FMT_*               long name
> >> +PCM_CODEC  (ALAW,         S16, alaw,         "PCM A-law / G.711 A-law");
> >> +PCM_DECODER(F16LE,        FLT, f16le,        "PCM 16.8 floating point
> >> little-endian");
> >
> > The labels don't align with the fields.
>
> I actually made it so that the field is below the middle of the label.
> You prefer the following I presume:
> //AV_CODEC_ID_*            pcm_* name
> //          AV_SAMPLE_FMT_*                  long name
> PCM_CODEC  (ALAW,         S16, alaw,         "PCM A-law / G.711 A-law");

I'd prefer if the first characters align.

> > And in the next patch ([PATCH 4/7] avcodec/pcm: Don't allocate LUT when
> > unused), with the introduction of PCM_CODEC_EXT(), the alignment of
> > PCM_CODEC/PCM_DECODER/PCM_CODEC_EXT is lost.
> >
>
> Only for a few codecs and only for the first field.

> Is this really so important?

No. I'd prefer the extra spaces to maintain alignment (especially when
there are only a few codecs that would be different), but I'm just
being extra nitpickish, same with the comment above. The patchset
looks good either way.

> I'd consider it worse if there were "PCM_CODEC    (" instead
> of "PCM_CODEC  (" everywhere else. (Of course, this point were moot if
> you had an idea for macro name shorter than PCM_CODEC_EXT.)

Ramiro