[MPlayer-dev-eng] [PATCH] Determine replay gain
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Sun Apr 2 18:41:02 EEST 2017
On Fri, Mar 31, 2017 at 04:17:45AM +0200, Ingo Brückl wrote:
> Index: libmpdemux/demux_audio.c
> ===================================================================
> --- libmpdemux/demux_audio.c (revision 37927)
> +++ libmpdemux/demux_audio.c (working copy)
> @@ -42,6 +42,9 @@
>
> #define HDR_SIZE 4
>
> +#define LAST_BITS(k, n) ((k) & ((1 << (n)) - 1))
> +#define MIDDLE_BITS(k, m, n) LAST_BITS((k) >> (m), ((n) - (m)))
> +
> typedef struct da_priv {
> int frmt;
> double next_pts;
> @@ -66,6 +69,8 @@
> struct mp3_hdr *next;
> } mp3_hdr_t;
>
> +static int r_gain;
> +
Shouldn't use global variables except for options (and even that
is not a very good idea).
> @@ -317,7 +322,29 @@
> data = stream_read_dword(s);
>
> if (data & 0x1) // frames field is present
> - return stream_read_dword(s); // frames
> + data = stream_read_dword(s); // frames
> +
> + if (stream_skip(s, 108)) {
I'm a bit worried if there's a risk of that seek breaking anything...
> + unsigned int dword;
> +
> + dword = stream_read_dword(s);
Merge the declaration and the assignment into one statement.
As a personal preference, I also rather write just "unsigned" instead of
"unsigned int".
But uint32_t might be a bit better anyway.
> + if (dword == MKBETAG('L','A','M','E') && stream_skip(s, 11)) {
> + uint32_t word;
> +
> + word = stream_read_word(s);
> +
> + /* Radio ReplayGain */
> + if (MIDDLE_BITS(word, 13, 15) == 1) {
> + r_gain = MIDDLE_BITS(word, 0, 8);
I wonder if we don't have functions to extract bits.
But even if we don't, I am not sure it's more readable than
((word >> 13) & 3) == 1
r_gain = word & 0xff; or r_gain = (uint8_t)word;
Especially since I never know if with such functions
the upper limit is inclusive or exclusive, so these
might all be off-by-one...
> + if (word & (1 << 9))
Maybe it's better readability to the average programmer,
but I actually prefer the (word >> 9) & 1 syntax nowadays,
as it doesn't suddenly start to fail if your bit position
is larger than 31 etc.
One last big thing: I am not very happy about adding demuxer
features that don't work with -demuxer lavf.
Especially since our demuxer are a bit problematic with metadata,
so I'd rather want use to have the option of suggesting -demuxer lavf
with breaking as few features as possible.
Regards,
Reimar
More information about the MPlayer-dev-eng
mailing list