Re: [PATCH] Determine replay gain

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
_______________________________________________
MPlayer-dev-eng mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng