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

[Zebediah Figura (she/her)]

On 5/4/21 2:25 PM, Connor McAdams wrote:
> 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.

To summarize an off-list conversation, I believe that GStreamer is doing 
something wrong here (or possibly we are) to cause this estimate to be 
incorrect. In particular, the specific numbers are off by a factor of 
exactly 8, which is more than a little suspicious. Ideally we should try 
to fix that first.

> 
>>>
>>>>>             /* 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.
>>>