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@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.