[PATCH 2/2] winegstreamer: Continuously query duration for mp3 audio streams.

[Connor McAdams]

On Tue, May 04, 2021 at 02:10:37PM -0500, Zebediah Figura (she/her) wrote:
> On 5/4/21 9:46 AM, Connor McAdams wrote:
> > On Mon, May 03, 2021 at 11:35:20AM -0500, Zebediah Figura (she/her) wrote:
> > > On 5/3/21 7:53 AM, Connor McAdams wrote:
> > > > For mp3 audio streams, continuously call gst_pad_query_duration until
> > > > the duration arrives. Fixes issue of incorrect song duration in Fallout:
> > > > New Vegas.
> > > > 
> > > > Signed-off-by: Connor McAdams <cmcadams at codeweavers.com>
> > > > ---
> > > >    dlls/winegstreamer/wg_parser.c | 27 +++++++++++++++++++++++++--
> > > >    1 file changed, 25 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c
> > > > index 1653701506c..1dae3bfb922 100644
> > > > --- a/dlls/winegstreamer/wg_parser.c
> > > > +++ b/dlls/winegstreamer/wg_parser.c
> > > > @@ -1504,13 +1504,22 @@ static gboolean src_event_cb(GstPad *pad, GstObject *parent, GstEvent *event)
> > > >        return ret;
> > > >    }
> > > > -static LONGLONG query_duration(GstPad *pad)
> > > > +static LONGLONG query_duration(GstPad *pad, gboolean query_duration)
> > > >    {
> > > >        gint64 duration, byte_length;
> > > >        if (gst_pad_query_duration(pad, GST_FORMAT_TIME, &duration))
> > > >            return duration / 100;
> > > > +    if (query_duration)
> > > > +    {
> > > > +        for (;;)
> > > > +        {
> > > > +            if (gst_pad_query_duration(pad, GST_FORMAT_TIME, &duration))
> > > > +                return duration / 100;
> > > > +        }
> > > > +    }
> > > > +
> > > 
> > > We don't want to risk an infinite loop here, or busy-wait. We should wait
> > > for duration-changed with a time-out.
> > > 
> > > We also should be calling query_duration when polling, rather than
> > > gst_pad_query_duration(). That is, this loop should be in
> > > wg_parser_connect().
> > > 
> > 
> > Using something like pthread_cond_timedwait? I was waiting on the
> > duration-changed event prior to my current solution of continuously
> > calling gst_pad_query_duration(), but it was still querying too early
> > and getting incorrect results. If we want to continuously call
> > query_duration(), should we have some way for it to return if it
> > converted from byte length and try again in that circumstance? It seems
> > that converting from byte length just doesn't work for this format.
> 
> I'm not sure I fully understand what you're referring here. Is there an
> element that is returning inaccurate results when we try to convert from
> byte length, but later accurate results when we request duration in time
> directly? How inaccurate is the initial guess?
>

Yes. In Fallout: New Vegas, the mp3 files that play on the radio return
an average length of about 30 seconds when most of the files themselves
are 3 minutes in length. After setting up the wait for duration-changed,
it became more accurate, but was still off by around 20-30 seconds for
duration. It was off enough that it causes the song to loop back over
again, and is undesirable.

> > 
> > > >        WARN("Failed to query time duration; trying to convert from byte length.\n");
> > > >        /* To accurately get a duration for the stream, we want to only consider the
> > > > @@ -1569,6 +1578,7 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s
> > > >        for (i = 0; i < parser->stream_count; ++i)
> > > >        {
> > > >            struct wg_parser_stream *stream = parser->streams[i];
> > > > +        gboolean duration_wait = FALSE;
> > > >            while (!stream->has_caps && !parser->error)
> > > >                pthread_cond_wait(&parser->init_cond, &parser->mutex);
> > > > @@ -1577,12 +1587,25 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s
> > > >                pthread_mutex_unlock(&parser->mutex);
> > > >                return E_FAIL;
> > > >            }
> > > > +
> > > > +        /*
> > > > +         * On MP3 audio streams, sometimes the duration value takes time to be
> > > > +         * set. In this case, we need to continuously call query_duration
> > > > +         * until we get it.
> > > > +         */
> > > > +        if (input_format.major_type == WG_MAJOR_TYPE_AUDIO &&
> > > > +            input_format.u.audio.format >= WG_AUDIO_FORMAT_MPEG1_LAYER1 &&
> > > > +            input_format.u.audio.format <= WG_AUDIO_FORMAT_MPEG1_LAYER3)
> > > > +        {
> > > > +            duration_wait = TRUE;
> > > > +        }
> > > > +
> > > 
> > > I don't think there's really a good reason to limit this to MPEG audio. If
> > > there are other demuxers (now or in the future, or even in the past) that
> > > force us to wait for duration, we want to do so.
> > > 
> > 
> > Good point, I can probably do something similar to how certain decoders
> > are blacklisted/disabled, and instead have certain decoders have a wait
> > duration wait behavior flag.
> 
> I don't think there's any reason to do that either. If an element reports
> duration immediately, we won't wait anyway. If it doesn't, we always want to
> wait.
>

Hm. The problem with this is, what constitues reporting duration? If
it's that we get a result from either gst_pad_query_duration in either
the time format or the byte-conversion, I don't think that'll work in
this circumstance. Attempting to get the length from the byte-conversion
here is just too inaccurate, making it almost useless. For this case, it
seems to only way to get accurate results is to wait for
gst_pad_query_duration with a format of GST_FORMAT_TIME. In that case,
query_duration() would have to have some special way of returning that
it got duration, but had to use byte-conversion to do so.

> > 
> > > >            /* GStreamer doesn't actually provide any guarantees about when duration
> > > >             * is available, even for seekable streams. However, many elements (e.g.
> > > >             * avidemux, wavparse, qtdemux) in practice record duration before
> > > >             * fixing caps, so as a heuristic, wait until we get caps before trying
> > > >             * to query for duration. */
> > > > -        stream->duration = query_duration(stream->their_src);
> > > > +        stream->duration = query_duration(stream->their_src, duration_wait);
> > > >        }
> > > >        pthread_mutex_unlock(&parser->mutex);
> > > > 
> > > 
> > 
> > Thank you very much for the review.
> >