Re: [PATCH v2 2/4] winemp3.acm: Disallow operations encoding to MPEG.

[Andrew Eikum]

I'd like this more with tests, showing what the application was doing
and how Wine failed its expectations but now meets them.

Andrew

On Fri, Jun 09, 2017 at 06:08:28PM -0500, Zebediah Figura wrote:
> Since the winemp3 driver is currently decode-only, and most Windows
> versions do not ship by default with a driver capable of encoding
> to mpeg3, we should report the driver's capabilities accordingly.
>
> v2: remove failing tests
> Signed-off-by: Zebediah Figura
>
> dlls/winemp3.acm/mpegl3.c | 80 ++++++++++-------------------------------------
>  1 file changed, 17 insertions(+), 63 deletions(-)
>
> diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> index cc6008da59e..c5d71bba524 100644
> --- a/dlls/winemp3.acm/mpegl3.c
> +++ b/dlls/winemp3.acm/mpegl3.c
> @@ -258,10 +258,13 @@ static	LRESULT	MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
>          mpg123_param(aad->mh, MPG123_ADD_FLAGS, MPG123_IGNORE_INFOFRAME, 0);
>  #endif
>      }
> - /* no encoding yet
> else if (adsi->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM &&
> -
>         adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3)
> - */
> -
>         (adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3 ||
> -
>          adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEG))
> - {
> -
>    WARN("Encoding to MPEG is not supported\n");
> -
>    goto theEnd;
> - }
> else goto theEnd;
> MPEG3_Reset(adsi, aad);
>
> @@ -736,54 +739,19 @@ static	LRESULT	MPEG3_FormatTagDetails(PACMFORMATTAGDETAILSW aftd, DWORD dwQuery)
>      case 1:
>    aftd->dwFormatTag = WAVE_FORMAT_MPEGLAYER3;
>    aftd->cbFormatSize = sizeof(MPEGLAYER3WAVEFORMAT);
> - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> -
>    aftd->cStandardFormats = 0;
>    lstrcpyW(aftd->szFormatTag, szMpeg3);
> break;
>    case 2:
>  aftd->dwFormatTag = WAVE_FORMAT_MPEG;
>  aftd->cbFormatSize = sizeof(MPEG1WAVEFORMAT);
> - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> -
>    aftd->cStandardFormats = 0;
>    lstrcpyW(aftd->szFormatTag, szMpeg);
> break;
>    }
>    return MMSYSERR_NOERROR;
>
> }
>
> -static void fill_in_mp3(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned bit_rate)
> -{
> - MPEGLAYER3WAVEFORMAT*   mp3wfx = (MPEGLAYER3WAVEFORMAT*)wfx;
> -
> - wfx->nAvgBytesPerSec = bit_rate / 8;
> - if (cbwfx >= sizeof(WAVEFORMATEX))
> -
>    wfx->cbSize = sizeof(MPEGLAYER3WAVEFORMAT) - sizeof(WAVEFORMATEX);
> - if (cbwfx >= sizeof(MPEGLAYER3WAVEFORMAT))
> - {
> -
>    mp3wfx->wID = MPEGLAYER3_ID_MPEG;
> -
>    mp3wfx->fdwFlags = MPEGLAYER3_FLAG_PADDING_OFF;
> -
>    mp3wfx->nBlockSize = (bit_rate * 144) / wfx->nSamplesPerSec;
> -
>    mp3wfx->nFramesPerBlock = 1;
> -
>    mp3wfx->nCodecDelay = 0x0571;
> - }
>
> -}
> -
>
> -static void fill_in_mpeg(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned bit_rate)
> -{
> - MPEG1WAVEFORMAT*   mp3wfx = (MPEG1WAVEFORMAT*)wfx;
> -
> - wfx->nAvgBytesPerSec = bit_rate / 8;
> - if (cbwfx >= sizeof(WAVEFORMATEX))
> -
>    wfx->cbSize = sizeof(MPEG1WAVEFORMAT) - sizeof(WAVEFORMATEX);
> - if (cbwfx >= sizeof(MPEG1WAVEFORMAT))
> - {
> -
>    mp3wfx->fwHeadLayer = ACM_MPEG_LAYER3;
> -
>    mp3wfx->dwHeadBitrate = wfx->nAvgBytesPerSec * 8;
> -
>    mp3wfx->fwHeadMode = ACM_MPEG_JOINTSTEREO;
> -
>    mp3wfx->fwHeadModeExt = 0xf;
> -
>    mp3wfx->wHeadEmphasis = 1;
> -
>    mp3wfx->fwHeadFlags = ACM_MPEG_ID_MPEG1;
> - }
>
> -}
> -
>
> /***********************************************************************
> -
>       MPEG3_FormatDetails
> -
>
> @@ -814,16 +782,8 @@ static	LRESULT	MPEG3_FormatDetails(PACMFORMATDETAILSW afd, DWORD dwQuery)
>        break;
>    case WAVE_FORMAT_MPEGLAYER3:
>    case WAVE_FORMAT_MPEG:
> -
>    if (afd->dwFormatIndex >= NUM_MPEG3_FORMATS) return ACMERR_NOTPOSSIBLE;
> -
>    afd->pwfx->nChannels = MPEG3_Formats[afd->dwFormatIndex].nChannels;
> -
>    afd->pwfx->nSamplesPerSec = MPEG3_Formats[afd->dwFormatIndex].rate;
> -
>    afd->pwfx->wBitsPerSample = MPEG3_Formats[afd->dwFormatIndex].nBits;
> -
>    afd->pwfx->nBlockAlign = 1;
> -
>    if (afd->dwFormatTag == WAVE_FORMAT_MPEGLAYER3)
> -
> fill_in_mp3(afd->cbwfx, afd->pwfx, 192000);
> -
>    else
> -
> fill_in_mpeg(afd->cbwfx, afd->pwfx, 192000);
> -
>    break;
> -
>        WARN("Encoding to MPEG is not supported\n");
> -
>        return ACMERR_NOTPOSSIBLE;
> default:
>            WARN("Unsupported tag %08x\n", afd->dwFormatTag);
>      return MMSYSERR_INVALPARAM;
>
> @@ -856,18 +816,15 @@ static	LRESULT	MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
>    adfs->pwfxDst->nChannels = adfs->pwfxSrc->nChannels;
>      if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_NSAMPLESPERSEC))
>          adfs->pwfxDst->nSamplesPerSec = adfs->pwfxSrc->nSamplesPerSec;
> - if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WBITSPERSAMPLE))
> - {
> - if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> -
>        adfs->pwfxDst->wBitsPerSample = 4;
> -
>    else
> -
>        adfs->pwfxDst->wBitsPerSample = 16;
> - }
> -
>    adfs->pwfxDst->wBitsPerSample = 16;
> if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WFORMATTAG))
>    {
>  if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> -
>        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_MPEGLAYER3;
> -
>    {
> -
>        WARN("Encoding to MPEG is not supported\n");
> -
>        return ACMERR_NOTPOSSIBLE;
> -
>    }
>    else
>        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_PCM;
> }
>
> @@ -883,12 +840,9 @@ static	LRESULT	MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
>          adfs->pwfxDst->nAvgBytesPerSec = adfs->pwfxDst->nSamplesPerSec * adfs->pwfxDst->nBlockAlign;
>          break;
>      case WAVE_FORMAT_MPEG:
> -
>    adfs->pwfxDst->nBlockAlign = 1;
> -
>    fill_in_mpeg(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> -
>    break;
> case WAVE_FORMAT_MPEGLAYER3:
> -
>    adfs->pwfxDst->nBlockAlign = 1;
> -
>    fill_in_mp3(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> -
>    WARN("Encoding to MPEG is not supported\n");
> -
>    return ACMERR_NOTPOSSIBLE;
>    break;
> default:
>        FIXME("\n");
>
> -- 
> 2.13.0

[PATCH v2 2/4] winemp3.acm: Disallow operations encoding to MPEG.

[Zebediah Figura]

On both of the Windows boxes I tested, the driver didn't list any MPEG 
formats from acmFormatEnum(), acmFormatDetails(), or acmFormatSuggest(), 
and listed a count of 0 for acmFormatTag() and acmFormatTagEnum(). 
sndrec32.exe therefore disallows converting or recording to mp3, and 
doesn't even show it in the list of options brought up by 
acmFormatChoose, hence the third patch in this series.

I had written some tests to corroborate this, but these tests didn't 
give consistent output [1], since presumably the drivers that exist on 
Windows vary, so I removed them. A better course of action would be to 
check if the format is decode-only, presumably via 
ACM_STREAMOPENF_QUERY, and then test if the above functions corroborate 
this.

Since I'd like to put some more tests on the PCM converter, and adding 
tests here would depend on those there, I'll hold off on resending this 
patch.

[1] https://source.winehq.org/patches/data/134748.testfail

On 06/12/2017 10:06 AM, Andrew Eikum wrote:
> I'd like this more with tests, showing what the application was doing
> and how Wine failed its expectations but now meets them.
>
> Andrew
>
> On Fri, Jun 09, 2017 at 06:08:28PM -0500, Zebediah Figura wrote:
> > Since the winemp3 driver is currently decode-only, and most Windows
> > versions do not ship by default with a driver capable of encoding
> > to mpeg3, we should report the driver's capabilities accordingly.
> >
> > v2: remove failing tests
> > Signed-off-by: Zebediah Figura
> >
> > dlls/winemp3.acm/mpegl3.c | 80 ++++++++++-------------------------------------
> >   1 file changed, 17 insertions(+), 63 deletions(-)
> >
> > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > index cc6008da59e..c5d71bba524 100644
> > --- a/dlls/winemp3.acm/mpegl3.c
> > +++ b/dlls/winemp3.acm/mpegl3.c
> > @@ -258,10 +258,13 @@ static	LRESULT	MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> >           mpg123_param(aad->mh, MPG123_ADD_FLAGS, MPG123_IGNORE_INFOFRAME, 0);
> >   #endif
> >       }
> > - /* no encoding yet
> >  else if (adsi->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM &&
> > -
> >         adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > - */
> > -
> >         (adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3 ||
> > -
> >          adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEG))
> > - {
> > -
> >    WARN("Encoding to MPEG is not supported\n");
> > -
> >    goto theEnd;
> > - }
> >  else goto theEnd;
> >  MPEG3_Reset(adsi, aad);
> >
> > @@ -736,54 +739,19 @@ static	LRESULT	MPEG3_FormatTagDetails(PACMFORMATTAGDETAILSW aftd, DWORD dwQuery)
> >       case 1:
> >    aftd->dwFormatTag = WAVE_FORMAT_MPEGLAYER3;
> >    aftd->cbFormatSize = sizeof(MPEGLAYER3WAVEFORMAT);
> > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > -
> >    aftd->cStandardFormats = 0;
> >     lstrcpyW(aftd->szFormatTag, szMpeg3);
> > break;
> >     case 2:
> >  aftd->dwFormatTag = WAVE_FORMAT_MPEG;
> >  aftd->cbFormatSize = sizeof(MPEG1WAVEFORMAT);
> > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > -
> >    aftd->cStandardFormats = 0;
> >     lstrcpyW(aftd->szFormatTag, szMpeg);
> > break;
> >     }
> >     return MMSYSERR_NOERROR;
> > }
> >
> > -static void fill_in_mp3(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned bit_rate)
> > -{
> > - MPEGLAYER3WAVEFORMAT*   mp3wfx = (MPEGLAYER3WAVEFORMAT*)wfx;
> > -
> > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > -
> >    wfx->cbSize = sizeof(MPEGLAYER3WAVEFORMAT) - sizeof(WAVEFORMATEX);
> > - if (cbwfx >= sizeof(MPEGLAYER3WAVEFORMAT))
> > - {
> > -
> >    mp3wfx->wID = MPEGLAYER3_ID_MPEG;
> > -
> >    mp3wfx->fdwFlags = MPEGLAYER3_FLAG_PADDING_OFF;
> > -
> >    mp3wfx->nBlockSize = (bit_rate * 144) / wfx->nSamplesPerSec;
> > -
> >    mp3wfx->nFramesPerBlock = 1;
> > -
> >    mp3wfx->nCodecDelay = 0x0571;
> > - }
> >
> > -}
> > -
> >
> > -static void fill_in_mpeg(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned bit_rate)
> > -{
> > - MPEG1WAVEFORMAT*   mp3wfx = (MPEG1WAVEFORMAT*)wfx;
> > -
> > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > -
> >    wfx->cbSize = sizeof(MPEG1WAVEFORMAT) - sizeof(WAVEFORMATEX);
> > - if (cbwfx >= sizeof(MPEG1WAVEFORMAT))
> > - {
> > -
> >    mp3wfx->fwHeadLayer = ACM_MPEG_LAYER3;
> > -
> >    mp3wfx->dwHeadBitrate = wfx->nAvgBytesPerSec * 8;
> > -
> >    mp3wfx->fwHeadMode = ACM_MPEG_JOINTSTEREO;
> > -
> >    mp3wfx->fwHeadModeExt = 0xf;
> > -
> >    mp3wfx->wHeadEmphasis = 1;
> > -
> >    mp3wfx->fwHeadFlags = ACM_MPEG_ID_MPEG1;
> > - }
> >
> > -}
> > - /***********************************************************************
> > -
> >       MPEG3_FormatDetails
> > -
> >
> > @@ -814,16 +782,8 @@ static	LRESULT	MPEG3_FormatDetails(PACMFORMATDETAILSW afd, DWORD dwQuery)
> >        break;
> >    case WAVE_FORMAT_MPEGLAYER3:
> >    case WAVE_FORMAT_MPEG:
> > -
> >    if (afd->dwFormatIndex >= NUM_MPEG3_FORMATS) return ACMERR_NOTPOSSIBLE;
> > -
> >    afd->pwfx->nChannels = MPEG3_Formats[afd->dwFormatIndex].nChannels;
> > -
> >    afd->pwfx->nSamplesPerSec = MPEG3_Formats[afd->dwFormatIndex].rate;
> > -
> >    afd->pwfx->wBitsPerSample = MPEG3_Formats[afd->dwFormatIndex].nBits;
> > -
> >    afd->pwfx->nBlockAlign = 1;
> > -
> >    if (afd->dwFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > -
> > fill_in_mp3(afd->cbwfx, afd->pwfx, 192000);
> > -
> >    else
> > -
> > fill_in_mpeg(afd->cbwfx, afd->pwfx, 192000);
> > -
> >    break;
> > -
> >        WARN("Encoding to MPEG is not supported\n");
> > -
> >        return ACMERR_NOTPOSSIBLE;
> > default:
> >             WARN("Unsupported tag %08x\n", afd->dwFormatTag);
> >      return MMSYSERR_INVALPARAM;
> >
> > @@ -856,18 +816,15 @@ static	LRESULT	MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> >    adfs->pwfxDst->nChannels = adfs->pwfxSrc->nChannels;
> >       if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_NSAMPLESPERSEC))
> >           adfs->pwfxDst->nSamplesPerSec = adfs->pwfxSrc->nSamplesPerSec;
> > -
> > if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WBITSPERSAMPLE))
> > - {
> > - if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > -
> >        adfs->pwfxDst->wBitsPerSample = 4;
> > -
> >    else
> > -
> >        adfs->pwfxDst->wBitsPerSample = 16;
> > - }
> > -
> >    adfs->pwfxDst->wBitsPerSample = 16;
> > if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WFORMATTAG))
> > {
> > if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > -
> >        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_MPEGLAYER3;
> > -
> >    {
> > -
> >        WARN("Encoding to MPEG is not supported\n");
> > -
> >        return ACMERR_NOTPOSSIBLE;
> > -
> >    }
> >     else
> >         adfs->pwfxDst->wFormatTag = WAVE_FORMAT_PCM;
> > }
> >
> > @@ -883,12 +840,9 @@ static	LRESULT	MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> >           adfs->pwfxDst->nAvgBytesPerSec = adfs->pwfxDst->nSamplesPerSec * adfs->pwfxDst->nBlockAlign;
> >           break;
> >       case WAVE_FORMAT_MPEG:
> > -
> >    adfs->pwfxDst->nBlockAlign = 1;
> > -
> >    fill_in_mpeg(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > -
> >    break;
> > case WAVE_FORMAT_MPEGLAYER3:
> > -
> >    adfs->pwfxDst->nBlockAlign = 1;
> > -
> >    fill_in_mp3(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > -
> >    WARN("Encoding to MPEG is not supported\n");
> > -
> >    return ACMERR_NOTPOSSIBLE;
> >     break;
> > default:
> >     FIXME("\n");
> >
> > -- 
> > 2.13.0

[PATCH v2 2/4] winemp3.acm: Disallow operations encoding to MPEG.

[Zebediah Figura]

Looking at this again I don't think there's really any way to test this; 
that is, there is no function that these "match" the output of. 
acmStreamConvert() with the QUERY option would be the obvious choice, 
and is basically the test case relevant to the application in question, 
but it requires a source format, and I'm not sure we can assume any 
given source format will work for all/any given MPEG destination format, 
even if converting to that MPEG format is possible in general. 
(waveInOpen() doesn't work either, and probably shouldn't be expected to.)

The goal of this patch is ultimately that MPEG doesn't show up in the 
list of formats brought up by acmFormatChoose, because we can't convert 
to it. If there's a better/more testable way to achieve this, please 
don't hesitate to suggest.

On 06/12/2017 02:10 PM, Zebediah Figura wrote:
> On both of the Windows boxes I tested, the driver didn't list any MPEG 
> formats from acmFormatEnum(), acmFormatDetails(), or acmFormatSuggest(), 
> and listed a count of 0 for acmFormatTag() and acmFormatTagEnum(). 
> sndrec32.exe therefore disallows converting or recording to mp3, and 
> doesn't even show it in the list of options brought up by 
> acmFormatChoose, hence the third patch in this series.
>
> I had written some tests to corroborate this, but these tests didn't 
> give consistent output [1], since presumably the drivers that exist on 
> Windows vary, so I removed them. A better course of action would be to 
> check if the format is decode-only, presumably via 
> ACM_STREAMOPENF_QUERY, and then test if the above functions corroborate 
> this.
>
> Since I'd like to put some more tests on the PCM converter, and adding 
> tests here would depend on those there, I'll hold off on resending this 
> patch.
>
> [1] https://source.winehq.org/patches/data/134748.testfail
>
> On 06/12/2017 10:06 AM, Andrew Eikum wrote:
> > I'd like this more with tests, showing what the application was doing
> > and how Wine failed its expectations but now meets them.
> >
> > Andrew
> >
> > On Fri, Jun 09, 2017 at 06:08:28PM -0500, Zebediah Figura wrote:
> > > Since the winemp3 driver is currently decode-only, and most Windows
> > > versions do not ship by default with a driver capable of encoding
> > > to mpeg3, we should report the driver's capabilities accordingly.
> > >
> > > v2: remove failing tests
> > > Signed-off-by: Zebediah Figura
> > >
> > > dlls/winemp3.acm/mpegl3.c | 80 
> > > ++++++++++-------------------------------------
> > >   1 file changed, 17 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > > index cc6008da59e..c5d71bba524 100644
> > > --- a/dlls/winemp3.acm/mpegl3.c
> > > +++ b/dlls/winemp3.acm/mpegl3.c
> > > @@ -258,10 +258,13 @@ static    LRESULT    
> > > MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> > >           mpg123_param(aad->mh, MPG123_ADD_FLAGS, 
> > > MPG123_IGNORE_INFOFRAME, 0);
> > >   #endif
> > >       }
> > > - /* no encoding yet
> > >  else if (adsi->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM &&
> > > -
> > >         adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > > - */
> > > -
> > >         (adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3 ||
> > > -
> > >          adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEG))
> > > - {
> > > -
> > >    WARN("Encoding to MPEG is not supported\n");
> > > -
> > >    goto theEnd;
> > > - }
> > >  else goto theEnd;
> > >  MPEG3_Reset(adsi, aad);
> > >
> > > @@ -736,54 +739,19 @@ static    LRESULT    
> > > MPEG3_FormatTagDetails(PACMFORMATTAGDETAILSW aftd, DWORD dwQuery)
> > >       case 1:
> > >       aftd->dwFormatTag = WAVE_FORMAT_MPEGLAYER3;
> > >       aftd->cbFormatSize = sizeof(MPEGLAYER3WAVEFORMAT);
> > > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > > -
> > >    aftd->cStandardFormats = 0;
> > >     lstrcpyW(aftd->szFormatTag, szMpeg3);
> > > break;
> > > case 2:
> > > aftd->dwFormatTag = WAVE_FORMAT_MPEG;
> > > aftd->cbFormatSize = sizeof(MPEG1WAVEFORMAT);
> > > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > > -
> > >    aftd->cStandardFormats = 0;
> > >     lstrcpyW(aftd->szFormatTag, szMpeg);
> > > break;
> > > }
> > > return MMSYSERR_NOERROR;
> > > }
> > >
> > > -static void fill_in_mp3(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned 
> > > bit_rate)
> > > -{
> > > - MPEGLAYER3WAVEFORMAT*   mp3wfx = (MPEGLAYER3WAVEFORMAT*)wfx;
> > > -
> > > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > > -
> > >    wfx->cbSize = sizeof(MPEGLAYER3WAVEFORMAT) - 
> > >
> > > sizeof(WAVEFORMATEX);
> > > - if (cbwfx >= sizeof(MPEGLAYER3WAVEFORMAT))
> > > - {
> > > -
> > >    mp3wfx->wID = MPEGLAYER3_ID_MPEG;
> > > -
> > >    mp3wfx->fdwFlags = MPEGLAYER3_FLAG_PADDING_OFF;
> > > -
> > >    mp3wfx->nBlockSize = (bit_rate * 144) / wfx->nSamplesPerSec;
> > > -
> > >    mp3wfx->nFramesPerBlock = 1;
> > > -
> > >    mp3wfx->nCodecDelay = 0x0571;
> > > - }
> > >
> > > -}
> > > -
> > >
> > > -static void fill_in_mpeg(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned 
> > > bit_rate)
> > > -{
> > > - MPEG1WAVEFORMAT*   mp3wfx = (MPEG1WAVEFORMAT*)wfx;
> > > -
> > > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > > -
> > >    wfx->cbSize = sizeof(MPEG1WAVEFORMAT) - sizeof(WAVEFORMATEX);
> > > - if (cbwfx >= sizeof(MPEG1WAVEFORMAT))
> > > - {
> > > -
> > >    mp3wfx->fwHeadLayer = ACM_MPEG_LAYER3;
> > > -
> > >    mp3wfx->dwHeadBitrate = wfx->nAvgBytesPerSec * 8;
> > > -
> > >    mp3wfx->fwHeadMode = ACM_MPEG_JOINTSTEREO;
> > > -
> > >    mp3wfx->fwHeadModeExt = 0xf;
> > > -
> > >    mp3wfx->wHeadEmphasis = 1;
> > > -
> > >    mp3wfx->fwHeadFlags = ACM_MPEG_ID_MPEG1;
> > > - }
> > >
> > > -}
> > > -
> > >
> > > /***********************************************************************
> > > -
> > >       MPEG3_FormatDetails
> > > -
> > >
> > > @@ -814,16 +782,8 @@ static    LRESULT    
> > > MPEG3_FormatDetails(PACMFORMATDETAILSW afd, DWORD dwQuery)
> > >           break;
> > >       case WAVE_FORMAT_MPEGLAYER3:
> > >       case WAVE_FORMAT_MPEG:
> > > -
> > >    if (afd->dwFormatIndex >= NUM_MPEG3_FORMATS) return 
> > >
> > > ACMERR_NOTPOSSIBLE;
> > > -
> > >    afd->pwfx->nChannels = 
> > >
> > > MPEG3_Formats[afd->dwFormatIndex].nChannels;
> > > -
> > >    afd->pwfx->nSamplesPerSec = 
> > >
> > > MPEG3_Formats[afd->dwFormatIndex].rate;
> > > -
> > >    afd->pwfx->wBitsPerSample = 
> > >
> > > MPEG3_Formats[afd->dwFormatIndex].nBits;
> > > -
> > >    afd->pwfx->nBlockAlign = 1;
> > > -
> > >    if (afd->dwFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > > -
> > >    fill_in_mp3(afd->cbwfx, afd->pwfx, 192000);
> > > -
> > >    else
> > > -
> > >    fill_in_mpeg(afd->cbwfx, afd->pwfx, 192000);
> > > -
> > >    break;
> > > -
> > >        WARN("Encoding to MPEG is not supported\n");
> > > -
> > >        return ACMERR_NOTPOSSIBLE;
> > > default:
> > >         WARN("Unsupported tag %08x\n", afd->dwFormatTag);
> > >     return MMSYSERR_INVALPARAM;
> > >
> > > @@ -856,18 +816,15 @@ static    LRESULT    
> > > MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> > >       adfs->pwfxDst->nChannels = adfs->pwfxSrc->nChannels;
> > >       if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_NSAMPLESPERSEC))
> > >           adfs->pwfxDst->nSamplesPerSec = adfs->pwfxSrc->nSamplesPerSec;
> > > -
> > > if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WBITSPERSAMPLE))
> > > - {
> > > - if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > > -
> > >        adfs->pwfxDst->wBitsPerSample = 4;
> > > -
> > >    else
> > > -
> > >        adfs->pwfxDst->wBitsPerSample = 16;
> > > - }
> > > -
> > >    adfs->pwfxDst->wBitsPerSample = 16;
> > > if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WFORMATTAG))
> > > {
> > > if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > > -
> > >        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_MPEGLAYER3;
> > > -
> > >    {
> > > -
> > >        WARN("Encoding to MPEG is not supported\n");
> > > -
> > >        return ACMERR_NOTPOSSIBLE;
> > > -
> > >    }
> > >     else
> > >         adfs->pwfxDst->wFormatTag = WAVE_FORMAT_PCM;
> > > }
> > >
> > > @@ -883,12 +840,9 @@ static    LRESULT    
> > > MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> > >           adfs->pwfxDst->nAvgBytesPerSec = 
> > > adfs->pwfxDst->nSamplesPerSec * adfs->pwfxDst->nBlockAlign;
> > >           break;
> > >       case WAVE_FORMAT_MPEG:
> > > -
> > >    adfs->pwfxDst->nBlockAlign = 1;
> > > -
> > >    fill_in_mpeg(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > > -
> > >    break;
> > > case WAVE_FORMAT_MPEGLAYER3:
> > > -
> > >    adfs->pwfxDst->nBlockAlign = 1;
> > > -
> > >    fill_in_mp3(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > > -
> > >    WARN("Encoding to MPEG is not supported\n");
> > > -
> > >    return ACMERR_NOTPOSSIBLE;
> > >     break;
> > > default:
> > >     FIXME("\n");
> > >
> > > -- 
> > > 2.13.0

[PATCH v2 2/4] winemp3.acm: Disallow operations encoding to MPEG.

[Andrew Eikum]

On Tue, Jun 20, 2017 at 06:04:45PM -0500, Zebediah Figura wrote:
> Looking at this again I don't think there's really any way to test this;
> that is, there is no function that these "match" the output of.
> acmStreamConvert() with the QUERY option would be the obvious choice, and is
> basically the test case relevant to the application in question, but it
> requires a source format, and I'm not sure we can assume any given source
> format will work for all/any given MPEG destination format, even if
> converting to that MPEG format is possible in general. (waveInOpen() doesn't
> work either, and probably shouldn't be expected to.)
>
> The goal of this patch is ultimately that MPEG doesn't show up in the list
> of formats brought up by acmFormatChoose, because we can't convert to it. If
> there's a better/more testable way to achieve this, please don't hesitate to
> suggest.

What does the application do? My understanding was it used
cStandardFormats to determine whether an encoding is available, and
then tried to open the ACM stream with that conversion, which would
then fail in Wine and cause the problem. Your patch fixed that by
changing cStandardFormats to 0, so the application never tried to open
the stream. Is that about right?

If so, you could add a test showing that if cStandardFormats > 0, then
the stream should open. If cStandardFormats == 0, then the stream
should fail to open.  Wine would fail that test without your patch,
but pass it with your patch. It's fine to limit the test case to the
specific source and destination formats in question here, there's no
need to be exhaustive.

Andrew
> On 06/12/2017 02:10 PM, Zebediah Figura wrote:
> > On both of the Windows boxes I tested, the driver didn't list any MPEG
> > formats from acmFormatEnum(), acmFormatDetails(), or acmFormatSuggest(),
> > and listed a count of 0 for acmFormatTag() and acmFormatTagEnum().
> > sndrec32.exe therefore disallows converting or recording to mp3, and
> > doesn't even show it in the list of options brought up by acmFormatChoose,
> > hence the third patch in this series.
> >
> > I had written some tests to corroborate this, but these tests didn't give
> > consistent output [1], since presumably the drivers that exist on Windows
> > vary, so I removed them. A better course of action would be to check if
> > the format is decode-only, presumably via ACM_STREAMOPENF_QUERY, and then
> > test if the above functions corroborate this.
> >
> > Since I'd like to put some more tests on the PCM converter, and adding
> > tests here would depend on those there, I'll hold off on resending this
> > patch.
> >
> > [1] https://source.winehq.org/patches/data/134748.testfail
> >
> > On 06/12/2017 10:06 AM, Andrew Eikum wrote:
> > > I'd like this more with tests, showing what the application was doing
> > > and how Wine failed its expectations but now meets them.
> > >
> > > Andrew
> > >
> > > On Fri, Jun 09, 2017 at 06:08:28PM -0500, Zebediah Figura wrote:
> > > > Since the winemp3 driver is currently decode-only, and most Windows
> > > > versions do not ship by default with a driver capable of encoding
> > > > to mpeg3, we should report the driver's capabilities accordingly.
> > > >
> > > > v2: remove failing tests
> > > > Signed-off-by: Zebediah Figura
> > > >
> > > > dlls/winemp3.acm/mpegl3.c | 80
> > > > ++++++++++-------------------------------------
> > > >  1 file changed, 17 insertions(+), 63 deletions(-)
> > > >
> > > > diff --git a/dlls/winemp3.acm/mpegl3.c b/dlls/winemp3.acm/mpegl3.c
> > > > index cc6008da59e..c5d71bba524 100644
> > > > --- a/dlls/winemp3.acm/mpegl3.c
> > > > +++ b/dlls/winemp3.acm/mpegl3.c
> > > > @@ -258,10 +258,13 @@ static    LRESULT
> > > > MPEG3_StreamOpen(PACMDRVSTREAMINSTANCE adsi)
> > > >          mpg123_param(aad->mh, MPG123_ADD_FLAGS,
> > > > MPG123_IGNORE_INFOFRAME, 0);
> > > >  #endif
> > > >      }
> > > > - /* no encoding yet
> > > > else if (adsi->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM &&
> > > > -
> > > >         adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > > > - */
> > > > -
> > > >         (adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEGLAYER3 ||
> > > > -
> > > >          adsi->pwfxDst->wFormatTag == WAVE_FORMAT_MPEG))
> > > > - {
> > > > -
> > > >    WARN("Encoding to MPEG is not supported\n");
> > > > -
> > > >    goto theEnd;
> > > > - }
> > > > else goto theEnd;
> > > > MPEG3_Reset(adsi, aad);
> > > >
> > > > @@ -736,54 +739,19 @@ static    LRESULT
> > > > MPEG3_FormatTagDetails(PACMFORMATTAGDETAILSW aftd, DWORD dwQuery)
> > > >      case 1:
> > > >      aftd->dwFormatTag = WAVE_FORMAT_MPEGLAYER3;
> > > >      aftd->cbFormatSize = sizeof(MPEGLAYER3WAVEFORMAT);
> > > > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > > > -
> > > >    aftd->cStandardFormats = 0;
> > > >    lstrcpyW(aftd->szFormatTag, szMpeg3);
> > > > break;
> > > >    case 2:
> > > >    aftd->dwFormatTag = WAVE_FORMAT_MPEG;
> > > >    aftd->cbFormatSize = sizeof(MPEG1WAVEFORMAT);
> > > > - aftd->cStandardFormats = NUM_MPEG3_FORMATS;
> > > > -
> > > >    aftd->cStandardFormats = 0;
> > > >    lstrcpyW(aftd->szFormatTag, szMpeg);
> > > > break;
> > > >    }
> > > >    return MMSYSERR_NOERROR;
> > > >
> > > > }
> > > > -static void fill_in_mp3(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned
> > > > bit_rate)
> > > > -{
> > > > - MPEGLAYER3WAVEFORMAT*   mp3wfx = (MPEGLAYER3WAVEFORMAT*)wfx;
> > > > -
> > > > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > > > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > > > -
> > > >    wfx->cbSize = sizeof(MPEGLAYER3WAVEFORMAT) -
> > > >
> > > > sizeof(WAVEFORMATEX);
> > > > - if (cbwfx >= sizeof(MPEGLAYER3WAVEFORMAT))
> > > > - {
> > > > -
> > > >    mp3wfx->wID = MPEGLAYER3_ID_MPEG;
> > > > -
> > > >    mp3wfx->fdwFlags = MPEGLAYER3_FLAG_PADDING_OFF;
> > > > -
> > > >    mp3wfx->nBlockSize = (bit_rate * 144) / wfx->nSamplesPerSec;
> > > > -
> > > >    mp3wfx->nFramesPerBlock = 1;
> > > > -
> > > >    mp3wfx->nCodecDelay = 0x0571;
> > > > - }
> > > >
> > > > -}
> > > > -
> > > >
> > > > -static void fill_in_mpeg(unsigned cbwfx, WAVEFORMATEX* wfx, unsigned
> > > > bit_rate)
> > > > -{
> > > > - MPEG1WAVEFORMAT*   mp3wfx = (MPEG1WAVEFORMAT*)wfx;
> > > > -
> > > > - wfx->nAvgBytesPerSec = bit_rate / 8;
> > > > - if (cbwfx >= sizeof(WAVEFORMATEX))
> > > > -
> > > >    wfx->cbSize = sizeof(MPEG1WAVEFORMAT) - sizeof(WAVEFORMATEX);
> > > > - if (cbwfx >= sizeof(MPEG1WAVEFORMAT))
> > > > - {
> > > > -
> > > >    mp3wfx->fwHeadLayer = ACM_MPEG_LAYER3;
> > > > -
> > > >    mp3wfx->dwHeadBitrate = wfx->nAvgBytesPerSec * 8;
> > > > -
> > > >    mp3wfx->fwHeadMode = ACM_MPEG_JOINTSTEREO;
> > > > -
> > > >    mp3wfx->fwHeadModeExt = 0xf;
> > > > -
> > > >    mp3wfx->wHeadEmphasis = 1;
> > > > -
> > > >    mp3wfx->fwHeadFlags = ACM_MPEG_ID_MPEG1;
> > > > - }
> > > >
> > > > -}
> > > > -
> > > >
> > > > /***********************************************************************
> > > > -
> > > >       MPEG3_FormatDetails
> > > > -
> > > >
> > > > @@ -814,16 +782,8 @@ static    LRESULT
> > > > MPEG3_FormatDetails(PACMFORMATDETAILSW afd, DWORD dwQuery)
> > > >          break;
> > > >      case WAVE_FORMAT_MPEGLAYER3:
> > > >      case WAVE_FORMAT_MPEG:
> > > > -
> > > >    if (afd->dwFormatIndex >= NUM_MPEG3_FORMATS) return
> > > >
> > > > ACMERR_NOTPOSSIBLE;
> > > > -
> > > >    afd->pwfx->nChannels =
> > > >
> > > > MPEG3_Formats[afd->dwFormatIndex].nChannels;
> > > > -
> > > >    afd->pwfx->nSamplesPerSec =
> > > >
> > > > MPEG3_Formats[afd->dwFormatIndex].rate;
> > > > -
> > > >    afd->pwfx->wBitsPerSample =
> > > >
> > > > MPEG3_Formats[afd->dwFormatIndex].nBits;
> > > > -
> > > >    afd->pwfx->nBlockAlign = 1;
> > > > -
> > > >    if (afd->dwFormatTag == WAVE_FORMAT_MPEGLAYER3)
> > > > -
> > > >    fill_in_mp3(afd->cbwfx, afd->pwfx, 192000);
> > > > -
> > > >    else
> > > > -
> > > >    fill_in_mpeg(afd->cbwfx, afd->pwfx, 192000);
> > > > -
> > > >    break;
> > > > -
> > > >        WARN("Encoding to MPEG is not supported\n");
> > > > -
> > > >        return ACMERR_NOTPOSSIBLE;
> > > > default:
> > > >            WARN("Unsupported tag %08x\n", afd->dwFormatTag);
> > > >        return MMSYSERR_INVALPARAM;
> > > >
> > > > @@ -856,18 +816,15 @@ static    LRESULT
> > > > MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> > > >      adfs->pwfxDst->nChannels = adfs->pwfxSrc->nChannels;
> > > >      if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_NSAMPLESPERSEC))
> > > >          adfs->pwfxDst->nSamplesPerSec = adfs->pwfxSrc->nSamplesPerSec;
> > > > - if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WBITSPERSAMPLE))
> > > > - {
> > > > - if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > > > -
> > > >        adfs->pwfxDst->wBitsPerSample = 4;
> > > > -
> > > >    else
> > > > -
> > > >        adfs->pwfxDst->wBitsPerSample = 16;
> > > > - }
> > > > -
> > > >    adfs->pwfxDst->wBitsPerSample = 16;
> > > > if (!(adfs->fdwSuggest & ACM_FORMATSUGGESTF_WFORMATTAG))
> > > >    {
> > > >    if (adfs->pwfxSrc->wFormatTag == WAVE_FORMAT_PCM)
> > > > -
> > > >        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_MPEGLAYER3;
> > > > -
> > > >    {
> > > > -
> > > >        WARN("Encoding to MPEG is not supported\n");
> > > > -
> > > >        return ACMERR_NOTPOSSIBLE;
> > > > -
> > > >    }
> > > >    else
> > > >        adfs->pwfxDst->wFormatTag = WAVE_FORMAT_PCM;
> > > > }
> > > >
> > > > @@ -883,12 +840,9 @@ static    LRESULT
> > > > MPEG3_FormatSuggest(PACMDRVFORMATSUGGEST adfs)
> > > >          adfs->pwfxDst->nAvgBytesPerSec =
> > > > adfs->pwfxDst->nSamplesPerSec * adfs->pwfxDst->nBlockAlign;
> > > >          break;
> > > >      case WAVE_FORMAT_MPEG:
> > > > -
> > > >    adfs->pwfxDst->nBlockAlign = 1;
> > > > -
> > > >    fill_in_mpeg(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > > > -
> > > >    break;
> > > > case WAVE_FORMAT_MPEGLAYER3:
> > > > -
> > > >    adfs->pwfxDst->nBlockAlign = 1;
> > > > -
> > > >    fill_in_mp3(adfs->cbwfxDst, adfs->pwfxDst, 192000);
> > > > -
> > > >    WARN("Encoding to MPEG is not supported\n");
> > > > -
> > > >    return ACMERR_NOTPOSSIBLE;
> > > >    break;
> > > > default:
> > > >        FIXME("\n");
> > > >
> > > > -- 
> > > > 2.13.0

[PATCH v2 2/4] winemp3.acm: Disallow operations encoding to MPEG.

[Zebediah Figura]

On 06/21/2017 07:18 AM, Andrew Eikum wrote:
> On Tue, Jun 20, 2017 at 06:04:45PM -0500, Zebediah Figura wrote:
> > Looking at this again I don't think there's really any way to test this;
> > that is, there is no function that these "match" the output of.
> > acmStreamConvert() with the QUERY option would be the obvious choice, and is
> > basically the test case relevant to the application in question, but it
> > requires a source format, and I'm not sure we can assume any given source
> > format will work for all/any given MPEG destination format, even if
> > converting to that MPEG format is possible in general. (waveInOpen() doesn't
> > work either, and probably shouldn't be expected to.)
> >
> > The goal of this patch is ultimately that MPEG doesn't show up in the list
> > of formats brought up by acmFormatChoose, because we can't convert to it. If
> > there's a better/more testable way to achieve this, please don't hesitate to
> > suggest.
>
> What does the application do? My understanding was it used
> cStandardFormats to determine whether an encoding is available, and
> then tried to open the ACM stream with that conversion, which would
> then fail in Wine and cause the problem. Your patch fixed that by
> changing cStandardFormats to 0, so the application never tried to open
> the stream. Is that about right?

Not directly, no. It calls acmFormatChoose, which brings up a 
system-defined dialog box allowing the user to choose from available 
formats to convert to. Then it tries to convert from whatever format it 
has open into whatever format acmFormatChoose returns; if this is MPEG, 
the latter will fail.