[PATCH 2/7] winegstreamer: Support MPEG-1 audio in wg_transform.

[Rémi Bernon]

On 5/3/22 23:18, Zebediah Figura (she/her) wrote:
> On 5/3/22 13:28, Anton Baskanov wrote:
>> Signed-off-by: Anton Baskanov <baskanov at gmail.com>
>> ---
>>   dlls/winegstreamer/wg_format.c    | 17 ++++++++++++++++-
>>   dlls/winegstreamer/wg_transform.c |  6 +++++-
>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/dlls/winegstreamer/wg_format.c 
>> b/dlls/winegstreamer/wg_format.c
>> index 608070b78e8..32d5a35e41a 100644
>> --- a/dlls/winegstreamer/wg_format.c
>> +++ b/dlls/winegstreamer/wg_format.c
>> @@ -331,6 +331,21 @@ static void 
>> wg_channel_mask_to_gst(GstAudioChannelPosition *positions, uint32_t
>>       }
>>   }
>> +static GstCaps *wg_format_to_caps_mpeg1_audio(const struct wg_format 
>> *format)
>> +{
>> +    GstCaps *caps;
>> +
>> +    if (!(caps = gst_caps_new_empty_simple("audio/mpeg")))
>> +        return NULL;
>> +
>> +    gst_caps_set_simple(caps, "mpegversion", G_TYPE_INT, 1, NULL);
>> +    gst_caps_set_simple(caps, "layer", G_TYPE_INT, 
>> format->u.mpeg1_audio.layer, NULL);
>> +    gst_caps_set_simple(caps, "rate", G_TYPE_INT, 
>> format->u.mpeg1_audio.rate, NULL);
>> +    gst_caps_set_simple(caps, "channels", G_TYPE_INT, 
>> format->u.mpeg1_audio.channels, NULL);
> 
> Should we set "parsed: true" here?
> 
>> +
>> +    return caps;
>> +}
>> +
>>   static GstCaps *wg_format_to_caps_audio(const struct wg_format *format)
>>   {
>>       GstAudioChannelPosition positions[32];
>> @@ -495,7 +510,7 @@ GstCaps *wg_format_to_caps(const struct wg_format 
>> *format)
>>           case WG_MAJOR_TYPE_UNKNOWN:
>>               return gst_caps_new_any();
>>           case WG_MAJOR_TYPE_MPEG1_AUDIO:
>> -            return NULL;
>> +            return wg_format_to_caps_mpeg1_audio(format);
>>           case WG_MAJOR_TYPE_WMA:
>>               return wg_format_to_caps_wma(format);
>>           case WG_MAJOR_TYPE_H264:
>> diff --git a/dlls/winegstreamer/wg_transform.c 
>> b/dlls/winegstreamer/wg_transform.c
>> index dca85e7366e..3af9822b016 100644
>> --- a/dlls/winegstreamer/wg_transform.c
>> +++ b/dlls/winegstreamer/wg_transform.c
>> @@ -114,6 +114,10 @@ static GstElement 
>> *transform_find_element(GstElementFactoryListType type, GstCap
>>       for (tmp = transforms; tmp != NULL && element == NULL; tmp = 
>> tmp->next)
>>       {
>>           name = 
>> gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data));
>> +        /* Ignore mpg123audiodec for now, as it has issues with frame 
>> delay,
>> +         * e.g. it does not output any data until it receives 2 
>> frames. */
>> +        if (!strcmp(name, "mpg123audiodec"))
>> +            continue;
> 
> I'd really rather not do this. Does this end up being a problem in 
> practice, and if so, how? (E.g. if all it does is break tests, I'd 
> rather just make the tests less restrictive, or mark things as todo_wine.)
> 
> Actually, I find this surprising anyway. IIRC mpg123 acted completely 
> synchronously when I wrote the mp3dmod tests, and just looking at the 
> GStreamer source I don't see any reason why mpg123audiodec wouldn't work 
> synchronously. (Maybe it's different for layer 3, but I can't imagine 
> why...)
> 

FWIW I intend to do a similar thing to disallow vaapi codecs for H264. 
They are prioritized over avdec_h264, when both are available, but they 
have issues and won't let us implement correct plane alignment easily.

It's a bit pointless to use them anyway as we don't support GPU buffers 
yet, and they could be re-enabled later when we do and for that case 
only, assuming the alignments are correct, or when they get fixed.

-- 
Rémi Bernon <rbernon at codeweavers.com>