Re: [PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Sebastian Lackner]

On 19.02.2017 19:54, Bruno Jesus wrote:
> From: Bruno Jesus <00cpxxx(a)gmail.com>
>
> Fixes bug https://bugs.winehq.org/show_bug.cgi?id=42483
>
> Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
> ---
>  dlls/winemp3.acm/mpegl3.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> index 28a1998..100aa49 100644
> --- a/dlls/winemp3.acm/mpegl3.c
> +++ b/dlls/winemp3.acm/mpegl3.c
> @@ -249,12 +249,14 @@ static	LRESULT	MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
>          aad->mh = mpg123_new(NULL,&err);
>          mpg123_open_feed(aad->mh);
>
> +#ifdef MPG123_IGNORE_INFOFRAME

Are you sure this works? On my system it is an enum, not a define.
> /* mpg123 may find a XING header in the mp3 and use that information
>           * to ask for seeks in order to read specific frames in the file.
>           * We cannot allow that since the caller application is feeding us.
>           * This fixes problems for mp3 files encoded with LAME (bug 42361)
>           */
>          mpg123_param(aad->mh, MPG123_ADD_FLAGS, MPG123_IGNORE_INFOFRAME, 0);
> +#endif
>      }
>      /* no encoding yet
>      else if (adsi->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM &&

[PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Bruno Jesus]

On Sun, Feb 19, 2017 at 3:57 PM, Sebastian Lackner <sebastian(a)fds-team.de>
wrote:
> On 19.02.2017 19:54, Bruno Jesus wrote:
> > From: Bruno Jesus <00cpxxx(a)gmail.com>
> >
> > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=42483
> >
> > Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
> > ---
> >  dlls/winemp3.acm/mpegl3.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > index 28a1998..100aa49 100644
> > --- a/dlls/winemp3.acm/mpegl3.c
> > +++ b/dlls/winemp3.acm/mpegl3.c
> > @@ -249,12 +249,14 @@ static  LRESULT MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE
> > adsi)
> >          aad->mh = mpg123_new(NULL,&err);
> >          mpg123_open_feed(aad->mh);
> >
> > +#ifdef MPG123_IGNORE_INFOFRAME
>
> Are you sure this works? On my system it is an enum, not a define.

You are absolutely right, it doesn't. I was a victim of my own "add error
inside #ifdef/else and recompile test", probably recompiled before saving.

[PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Bruno Jesus]

On Sun, Feb 19, 2017 at 4:04 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
> On Sun, Feb 19, 2017 at 3:57 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
> > On 19.02.2017 19:54, Bruno Jesus wrote:
> > > From: Bruno Jesus <00cpxxx(a)gmail.com>
> > >
> > > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=42483
> > >
> > > Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
> > > ---
> > >  dlls/winemp3.acm/mpegl3.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > > index 28a1998..100aa49 100644
> > > --- a/dlls/winemp3.acm/mpegl3.c
> > > +++ b/dlls/winemp3.acm/mpegl3.c
> > > @@ -249,12 +249,14 @@ static  LRESULT MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> > >          aad->mh = mpg123_new(NULL,&err);
> > >          mpg123_open_feed(aad->mh);
> > >
> > > +#ifdef MPG123_IGNORE_INFOFRAME
> >
> > Are you sure this works? On my system it is an enum, not a define.
>
> You are absolutely right, it doesn't. I was a victim of my own "add error inside #ifdef/else and recompile test", probably recompiled before saving.

Would you mind helping me understand why the check at [1] fails with:

14734 configure:13848: checking for MPG123_IGNORE_INFOFRAME
14735 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
14736 configure:13848: $? = 0
14737 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
14738 configure:13848: $? = 0
14739 configure:13848: result: no

[1] http://pastebin.com/nveZQNDR

I never did this sort of change before and the best example I found
was some libxml2 stuff inside configure.ac.

Thanks in advance,
Bruno

[PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Sebastian Lackner]

On 19.02.2017 20:25, Bruno Jesus wrote:
> On Sun, Feb 19, 2017 at 4:04 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
> > On Sun, Feb 19, 2017 at 3:57 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
> > > On 19.02.2017 19:54, Bruno Jesus wrote:
> > > > From: Bruno Jesus <00cpxxx(a)gmail.com>
> > > >
> > > > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=42483
> > > >
> > > > Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
> > > > ---
> > > >  dlls/winemp3.acm/mpegl3.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > > > index 28a1998..100aa49 100644
> > > > --- a/dlls/winemp3.acm/mpegl3.c
> > > > +++ b/dlls/winemp3.acm/mpegl3.c
> > > > @@ -249,12 +249,14 @@ static  LRESULT MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> > > >          aad->mh = mpg123_new(NULL,&err);
> > > >          mpg123_open_feed(aad->mh);
> > > >
> > > > +#ifdef MPG123_IGNORE_INFOFRAME
> > >
> > > Are you sure this works? On my system it is an enum, not a define.
> >
> > You are absolutely right, it doesn't. I was a victim of my own "add error inside #ifdef/else and recompile test", probably recompiled before saving.
>
> Would you mind helping me understand why the check at [1] fails with:
>
> 14734 configure:13848: checking for MPG123_IGNORE_INFOFRAME
> 14735 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> 14736 configure:13848: $? = 0
> 14737 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> 14738 configure:13848: $? = 0
> 14739 configure:13848: result: no
>
> [1] http://pastebin.com/nveZQNDR
>
> I never did this sort of change before and the best example I found
> was some libxml2 stuff inside configure.ac.
>
> Thanks in advance,
> Bruno

Usually config.log also contains the code which was used for the check.
In this case, I would assume it fails because MPG123_IGNORE_INFOFRAME
isn't really a type. The type is "enum mpg123_param_flags", but testing
for that obviously doesn't make much sense.

I would suggest a slightly different solution. The commit where the
feature (including the MPG123_IGNORE_INFOFRAME const) was added is:

https://www.mpg123.de/cgi-bin/scm/mpg123?view=revision&revision=2910
https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/src/libmpg123/mpg123.h.in?r1=...

As you can see, in the same commit the API_VERSION was bumped to 31.

https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/configure.ac?r1=2910&r2=2909&...

So why not just something like:

#if MPG123_API_VERSION >= 31 /* needed for MPG123_IGNORE_FRAMEINFO */

Best regards,
Sebastian

[PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Bruno Jesus]

On Sun, Feb 19, 2017 at 5:05 PM, Sebastian Lackner
<sebastian(a)fds-team.de> wrote:
> On 19.02.2017 20:25, Bruno Jesus wrote:
> > Would you mind helping me understand why the check at [1] fails with:
> >
> > 14734 configure:13848: checking for MPG123_IGNORE_INFOFRAME
> > 14735 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> > 14736 configure:13848: $? = 0
> > 14737 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> > 14738 configure:13848: $? = 0
> > 14739 configure:13848: result: no
> >
> > [1] http://pastebin.com/nveZQNDR
> >
> > I never did this sort of change before and the best example I found
> > was some libxml2 stuff inside configure.ac.
>
> Usually config.log also contains the code which was used for the check.
> In this case, I would assume it fails because MPG123_IGNORE_INFOFRAME
> isn't really a type. The type is "enum mpg123_param_flags", but testing
> for that obviously doesn't make much sense.

Usually there is the code, but not in this case. I don't know why.
> I would suggest a slightly different solution. The commit where the
> feature (including the MPG123_IGNORE_INFOFRAME const) was added is:
>
> https://www.mpg123.de/cgi-bin/scm/mpg123?view=revision&revision=2910
> https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/src/libmpg123/mpg123.h.in?r1=...
>
> As you can see, in the same commit the API_VERSION was bumped to 31.
>
> https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/configure.ac?r1=2910&r2=2909&...
>
> So why not just something like:
>
> #if MPG123_API_VERSION >= 31 /* needed for MPG123_IGNORE_FRAMEINFO */

Looks perfect to me, thanks =)

[PATCH] winemp3.acm: Fixes compilation for older mpg123 library versions

[Sebastian Lackner]

On 19.02.2017 21:05, Sebastian Lackner wrote:
> On 19.02.2017 20:25, Bruno Jesus wrote:
> > On Sun, Feb 19, 2017 at 4:04 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote:
> > > On Sun, Feb 19, 2017 at 3:57 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
> > > > On 19.02.2017 19:54, Bruno Jesus wrote:
> > > > > From: Bruno Jesus <00cpxxx(a)gmail.com>
> > > > >
> > > > > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=42483
> > > > >
> > > > > Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com>
> > > > > ---
> > > > >  dlls/winemp3.acm/mpegl3.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > > > > index 28a1998..100aa49 100644
> > > > > --- a/dlls/winemp3.acm/mpegl3.c
> > > > > +++ b/dlls/winemp3.acm/mpegl3.c
> > > > > @@ -249,12 +249,14 @@ static  LRESULT MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> > > > >          aad->mh = mpg123_new(NULL,&err);
> > > > >          mpg123_open_feed(aad->mh);
> > > > >
> > > > > +#ifdef MPG123_IGNORE_INFOFRAME
> > > >
> > > > Are you sure this works? On my system it is an enum, not a define.
> > >
> > > You are absolutely right, it doesn't. I was a victim of my own "add error inside #ifdef/else and recompile test", probably recompiled before saving.
> >
> > Would you mind helping me understand why the check at [1] fails with:
> >
> > 14734 configure:13848: checking for MPG123_IGNORE_INFOFRAME
> > 14735 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> > 14736 configure:13848: $? = 0
> > 14737 configure:13848: ccache gcc-4.6 -m32 -c -g -O0   conftest.c >&5
> > 14738 configure:13848: $? = 0
> > 14739 configure:13848: result: no
> >
> > [1] http://pastebin.com/nveZQNDR
> >
> > I never did this sort of change before and the best example I found
> > was some libxml2 stuff inside configure.ac.
> >
> > Thanks in advance,
> > Bruno
>
> Usually config.log also contains the code which was used for the check.
> In this case, I would assume it fails because MPG123_IGNORE_INFOFRAME
> isn't really a type. The type is "enum mpg123_param_flags", but testing
> for that obviously doesn't make much sense.
>
> I would suggest a slightly different solution. The commit where the
> feature (including the MPG123_IGNORE_INFOFRAME const) was added is:
>
> https://www.mpg123.de/cgi-bin/scm/mpg123?view=revision&revision=2910
> https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/src/libmpg123/mpg123.h.in?r1=...
>
> As you can see, in the same commit the API_VERSION was bumped to 31.
>
> https://www.mpg123.de/cgi-bin/scm/mpg123/trunk/configure.ac?r1=2910&r2=2909&...
>
> So why not just something like:
>
> #if MPG123_API_VERSION >= 31 /* needed for MPG123_IGNORE_FRAMEINFO */
>
> Best regards,
> Sebastian

I just checked, and Ubuntu Precise ships header files with:
"#define MPG123_API_VERSION 25"
So such a solution would indeed fix the issue. Alternatively you could
of course also define the missing enum for API_VERSION < 31.