On Sat, Mar 15, 2025 at 5:56 AM Andreas Rheinhardt
<andreas.rheinha...@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
_______________________________________________
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".
