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

[Anton Baskanov]

On 04.05.2022 04:18, Zebediah Figura (she/her) wrote:
> On 5/3/22 13:28, Anton Baskanov wrote:
> > Signed-off-by: Anton Baskanov
> >
> > 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?

Fixed in v2.
> > -
> > - 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...)

There are two issues, actually:
* mpg123 itself doesn't produce any output until it gets 2 frames of
   input data. Then it decodes both of them, though.
* mpg123audiodec calls mpg123_decode_frame() only once per input frame,
   so the call sequence looks like this:

mpg123_feed(frame 0)
   mpg123_decode_frame() -> MPG123_NEED_MORE
   mpg123_feed(frame 1)
   mpg123_decode_frame() -> MPG123_NEW_FORMAT
   mpg123_feed(frame 2)
   mpg123_decode_frame() -> MPG123_OK (frame 0)
   mpg123_feed(frame 3)
   mpg123_decode_frame() -> MPG123_OK (frame 1)

This causes the output to always be 2 frames behind the input.

This should not be a problem in practice, as we get all the data 
eventually, except for the last 2 frames (to fix these we'd need to send 
a buffer with the DISCONT flag set in EndOfStream()).

I've fixed the tests and removed this check in v2.
> >        if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL)))
> >            GST_WARNING("Failed to create %s element.", name);
> >    }
> >
> > @@ -224,6 +228,7 @@ NTSTATUS wg_transform_create(void *args)
> >                 */
> >                transform->input_max_length = 16;
> >                /* fallthrough */
> > -
> >    case WG_MAJOR_TYPE_MPEG1_AUDIO:
> >      case WG_MAJOR_TYPE_WMA:
> >          if (!(element = transform_find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, raw_caps))
> >                  || !transform_append_element(transform, element, &first, &last))
> >
> > @@ -233,7 +238,6 @@ NTSTATUS wg_transform_create(void *args)
> >                }
> >                break;
> > -
> >    case WG_MAJOR_TYPE_MPEG1_AUDIO:
> >      case WG_MAJOR_TYPE_AUDIO:
> >      case WG_MAJOR_TYPE_VIDEO:
> >      case WG_MAJOR_TYPE_UNKNOWN: