[PATCH 1/5] winegstreamer: Inline query_duration() into wg_parser_connect().
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/winegstreamer/wg_parser.c | 44 ++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 0d59297a026..acea68ad27d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1496,26 +1496,6 @@ static gboolean src_event_cb(GstPad *pad, GstObject *parent, GstEvent *event) return ret; } -static LONGLONG query_duration(GstPad *pad) -{ - gint64 duration, byte_length; - - if (gst_pad_query_duration(pad, GST_FORMAT_TIME, &duration)) - return duration / 100; - - 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 - * length of that stream. Hence, query for the pad duration, instead of - * using the file duration. */ - if (gst_pad_query_duration(pad, GST_FORMAT_BYTES, &byte_length) - && gst_pad_query_convert(pad, GST_FORMAT_BYTES, byte_length, GST_FORMAT_TIME, &duration)) - return duration / 100; - - ERR("Failed to query duration.\n"); - return 0; -} - static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_size) { GstStaticPadTemplate src_template = GST_STATIC_PAD_TEMPLATE("quartz_src", @@ -1552,6 +1532,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]; + gint64 duration, byte_length; while (!stream->has_caps && !parser->error) pthread_cond_wait(&parser->init_cond, &parser->mutex); @@ -1565,7 +1546,28 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s * 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); + if (gst_pad_query_duration(stream->their_src, GST_FORMAT_TIME, &duration)) + { + stream->duration = duration / 100; + } + else + { + 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 + * length of that stream. Hence, query for the pad duration, instead of + * using the file duration. */ + if (gst_pad_query_duration(stream->their_src, GST_FORMAT_BYTES, &byte_length) + && gst_pad_query_convert(stream->their_src, GST_FORMAT_BYTES, byte_length, + GST_FORMAT_TIME, &duration)) + { + stream->duration = duration / 100; + } + else + { + ERR("Failed to query duration.\n"); + } + } } pthread_mutex_unlock(&parser->mutex); -- 2.25.1
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51126 Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/winegstreamer/wg_parser.c | 59 ++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index acea68ad27d..49823508568 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1536,37 +1536,46 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s while (!stream->has_caps && !parser->error) pthread_cond_wait(&parser->init_cond, &parser->mutex); - if (parser->error) + /* For some GStreamer elements we have to wait for duration-changed + * before querying for duration. However, elements that provide + * duration immediatly don't sent this message, so try to query first + * and then wait if the query fails. */ + for (;;) { - pthread_mutex_unlock(&parser->mutex); - return E_FAIL; - } - /* 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. */ - if (gst_pad_query_duration(stream->their_src, GST_FORMAT_TIME, &duration)) - { - stream->duration = duration / 100; - } - else - { - 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 - * length of that stream. Hence, query for the pad duration, instead of - * using the file duration. */ - if (gst_pad_query_duration(stream->their_src, GST_FORMAT_BYTES, &byte_length) - && gst_pad_query_convert(stream->their_src, GST_FORMAT_BYTES, byte_length, - GST_FORMAT_TIME, &duration)) + if (parser->error) + { + pthread_mutex_unlock(&parser->mutex); + return E_FAIL; + } + if (gst_pad_query_duration(stream->their_src, GST_FORMAT_TIME, &duration)) { stream->duration = duration / 100; + break; } - else + /* GStreamer versions before 1.5.90 fail to compute the duration of + * short MP3 files without Xing or VBRI headers, but it's still + * possible to get an estimated duration by converting from byte + * length. See <gstreamer.git:6d78d32d51970003d7>. */ + if (stream->eos) { - ERR("Failed to query duration.\n"); + 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 + * length of that stream. Hence, query for the pad duration, instead of + * using the file duration. */ + if (gst_pad_query_duration(stream->their_src, GST_FORMAT_BYTES, &byte_length) + && gst_pad_query_convert(stream->their_src, GST_FORMAT_BYTES, byte_length, + GST_FORMAT_TIME, &duration)) + { + stream->duration = duration / 100; + } + else + { + ERR("Failed to query duration.\n"); + } + break; } + pthread_cond_wait(&parser->init_cond, &parser->mutex); } } -- 2.25.1
On 6/21/21 9:35 AM, Anton Baskanov wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51126 Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/winegstreamer/wg_parser.c | 59 ++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-)
I'm not opposed to something like this patch set in principle, but from what I recall the last time this bug was discussed, we were getting durations from GStreamer that were off by a factor of exactly 8, suggesting that there was a bug elsewhere. Has that been investigated?
Yes, the issue is that we are passing the source file size to a convert query that expects decoded data size. With the right bitrate, we can get this factor of 8 (e.g. 192 kbps, 48 kHz, stereo MP3 has a compression ratio of 8:1). Patch 5/5 fixes this. On понедельник, 21 июня 2021 г. 22:35:07 +07 you wrote:
On 6/21/21 9:35 AM, Anton Baskanov wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51126 Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> ---
dlls/winegstreamer/wg_parser.c | 59 ++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-)
I'm not opposed to something like this patch set in principle, but from what I recall the last time this bug was discussed, we were getting durations from GStreamer that were off by a factor of exactly 8, suggesting that there was a bug elsewhere. Has that been investigated?
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/winegstreamer/wg_parser.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 49823508568..98a53f69132 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1575,7 +1575,21 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s } break; } - pthread_cond_wait(&parser->init_cond, &parser->mutex); + /* Elements based on GstBaseParse send duration-changed before + * actually updating the duration in GStreamer versions prior + * to 1.17.1. See <gstreamer.git:d28e0b4147fe7073b2>. So after + * receiving duration-changed we have to continue polling until + * the query succeeds. */ + if (parser->has_duration) + { + pthread_mutex_unlock(&parser->mutex); + g_usleep(10000); + pthread_mutex_lock(&parser->mutex); + } + else + { + pthread_cond_wait(&parser->init_cond, &parser->mutex); + } } } -- 2.25.1
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- dlls/winegstreamer/wg_parser.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 98a53f69132..4ee8d7198e4 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1804,15 +1804,6 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser) return FALSE; } - pthread_mutex_lock(&parser->mutex); - while (!parser->has_duration && !parser->error && !stream->eos) - pthread_cond_wait(&parser->init_cond, &parser->mutex); - if (parser->error) - { - pthread_mutex_unlock(&parser->mutex); - return FALSE; - } - pthread_mutex_unlock(&parser->mutex); return TRUE; } -- 2.25.1
Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> --- Seems like GST_FORMAT_BYTES duration queries are forwarded all the way up to my_src, which returns the source file size. On the other hand, the convert query on stream->their_src expects the size of the decoded raw data. This patch uses parser->their_sink to convert the source file size to get a correct duration estimate. --- dlls/winegstreamer/wg_parser.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 4ee8d7198e4..f0a967cb919 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1532,7 +1532,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]; - gint64 duration, byte_length; + gint64 duration; while (!stream->has_caps && !parser->error) pthread_cond_wait(&parser->init_cond, &parser->mutex); @@ -1560,12 +1560,8 @@ static HRESULT CDECL wg_parser_connect(struct wg_parser *parser, uint64_t file_s { 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 - * length of that stream. Hence, query for the pad duration, instead of - * using the file duration. */ - if (gst_pad_query_duration(stream->their_src, GST_FORMAT_BYTES, &byte_length) - && gst_pad_query_convert(stream->their_src, GST_FORMAT_BYTES, byte_length, - GST_FORMAT_TIME, &duration)) + if (gst_pad_query_convert(parser->their_sink, GST_FORMAT_BYTES, file_size, + GST_FORMAT_TIME, &duration)) { stream->duration = duration / 100; } -- 2.25.1
On 6/21/21 9:35 AM, Anton Baskanov wrote:
Seems like GST_FORMAT_BYTES duration queries are forwarded all the way up to my_src, which returns the source file size. On the other hand, the convert query on stream->their_src expects the size of the decoded raw data. This patch uses parser->their_sink to convert the source file size to get a correct duration estimate.
Sorry for the complete silence. This patch set, and this patch in particular, has just been sitting there bothering me. I suspect that even if this patch set is correct it needs an audit of GStreamer elements to make sure it's always doing the right thing. The basic problem is twofold: firstly, that GStreamer duration and convert queries are underspecified; and secondly, that the apparent actual behaviour is counterintuitive. For example, I'd expect that a GST_FORMAT_TIME duration query should retrieve the actual running time of a stream (which might be different from the running time of a muxed file!), and that a GST_FORMAT_BYTES duration query should retrieve the total *output* size, in bytes, of a stream. But the default behaviour for any pad is to cheerfully ask the upstream filter, and GstBaseParse even explicitly does this *before* returning its own answers, so it'll end up returning the encoded size or even the whole (possibly muxed) file size. A similar problem occurs with GST_QUERY_CONVERT. The ultimate effect is that the application can't predict who will respond to a duration or convert query, and thus they have no idea how to interpret the data they get back. In the case of DURATION + GST_FORMAT_TIME I suspect most applications get away with it, because usually the duration of a stream is (or is close enough to) the duration of a file. But GST_FORMAT_BYTES and any variation of CONVERT just seems completely broken... Unfortunately, I'm not sure that downstream CONVERT queries are any better. The default behaviour is still to pass the query to a random src, and the first two demuxers I checked (avidemux and mpegpsdemux) both do that. I think both actually give us a correct duration in response to GST_QUERY_DURATION + GST_FORMAT_TIME, but it's not a good sign. It may end up being best just to revert this path entirely, and rely on polling for GST_QUERY_DURATION + GST_FORMAT_TIME. The path was added by 613446d018. Looking back at mail history, I think I wrote it in response to [1], so it was actually just for dlls/quartz/tests/test.mp3. It turns out I only got away with this because, in the case of mp3, there's no container, so the byte length of the encoded stream is the byte length of the file, mpg123 just happened to handle neither of these queries, and mpegaudioparse handled GST_QUERY_CONVERT in the intuitive way. Unfortunately, unless I misread, I think there's currently no reliable way to get the duration of a CBR file from mpegaudioparse, except by relying on GST_QUERY_CONVERT. So, what do we do going forward? Unfortunately I think this is the point where we need to ask GStreamer for help, ideally to clarify their documentation, and perhaps fix elements accordingly. And if we can't reliably use GST_QUERY_CONVERT, we may want to revert 613446d018 entirely, and try to fix mpegaudioparse to give us a sane duration. And the latter seems like probably a good idea anyway. The rest of this series looks mostly correct, but patch 2/5 is tricky, because it's also not clear to me from the GStreamer documentation whether duration-changed will always be sent if the duration isn't immediately available. And if we need to ask for clarification anyway, it would probably be good to get that clarified at the same time. Anyway, I'll start writing an email to gstreamer-devel soon. ἔρρωσθε, Zebediah [1] https://www.winehq.org/pipermail/wine-devel/2020-June/167776.html
On пятница, 2 июля 2021 г. 04:05:12 +07 you wrote:
On 6/21/21 9:35 AM, Anton Baskanov wrote:
Seems like GST_FORMAT_BYTES duration queries are forwarded all the way up to my_src, which returns the source file size. On the other hand, the convert query on stream->their_src expects the size of the decoded raw data. This patch uses parser->their_sink to convert the source file size to get a correct duration estimate.
Sorry for the complete silence. This patch set, and this patch in particular, has just been sitting there bothering me. I suspect that even if this patch set is correct it needs an audit of GStreamer elements to make sure it's always doing the right thing.
The basic problem is twofold: firstly, that GStreamer duration and convert queries are underspecified; and secondly, that the apparent actual behaviour is counterintuitive.
For example, I'd expect that a GST_FORMAT_TIME duration query should retrieve the actual running time of a stream (which might be different from the running time of a muxed file!), and that a GST_FORMAT_BYTES duration query should retrieve the total *output* size, in bytes, of a stream. But the default behaviour for any pad is to cheerfully ask the upstream filter, and GstBaseParse even explicitly does this *before* returning its own answers, so it'll end up returning the encoded size or even the whole (possibly muxed) file size. A similar problem occurs with GST_QUERY_CONVERT.
The ultimate effect is that the application can't predict who will respond to a duration or convert query, and thus they have no idea how to interpret the data they get back. In the case of DURATION + GST_FORMAT_TIME I suspect most applications get away with it, because usually the duration of a stream is (or is close enough to) the duration of a file. But GST_FORMAT_BYTES and any variation of CONVERT just seems completely broken...
Unfortunately, I'm not sure that downstream CONVERT queries are any better. The default behaviour is still to pass the query to a random src, and the first two demuxers I checked (avidemux and mpegpsdemux) both do that. I think both actually give us a correct duration in response to GST_QUERY_DURATION + GST_FORMAT_TIME, but it's not a good sign.
It may end up being best just to revert this path entirely, and rely on polling for GST_QUERY_DURATION + GST_FORMAT_TIME. The path was added by 613446d018. Looking back at mail history, I think I wrote it in response to [1], so it was actually just for dlls/quartz/tests/test.mp3.
It turns out I only got away with this because, in the case of mp3, there's no container, so the byte length of the encoded stream is the byte length of the file, mpg123 just happened to handle neither of these queries, and mpegaudioparse handled GST_QUERY_CONVERT in the intuitive way.
Unfortunately, unless I misread, I think there's currently no reliable way to get the duration of a CBR file from mpegaudioparse, except by relying on GST_QUERY_CONVERT.
AFAICS, mpegaudioparse should be able to get the duration of CBR files just fine since GstBaseParse provides a duration estimate every 50 frames until a subclass sets the actual duration. The only case where we have to fall back to GST_QUERY_CONVERT is a very short CBR file with GStreamer versions before 1.5.90. I don't know if these old versions are still relevant, though.
So, what do we do going forward? Unfortunately I think this is the point where we need to ask GStreamer for help, ideally to clarify their documentation, and perhaps fix elements accordingly. And if we can't reliably use GST_QUERY_CONVERT, we may want to revert 613446d018 entirely, and try to fix mpegaudioparse to give us a sane duration. And the latter seems like probably a good idea anyway.
The rest of this series looks mostly correct, but patch 2/5 is tricky, because it's also not clear to me from the GStreamer documentation whether duration-changed will always be sent if the duration isn't immediately available. And if we need to ask for clarification anyway, it would probably be good to get that clarified at the same time.
Anyway, I'll start writing an email to gstreamer-devel soon.
ἔρρωσθε, Zebediah
[1] https://www.winehq.org/pipermail/wine-devel/2020-June/167776.html
On 7/2/21 1:27 PM, Anton Baskanov wrote:
On пятница, 2 июля 2021 г. 04:05:12 +07 you wrote:
On 6/21/21 9:35 AM, Anton Baskanov wrote:
Seems like GST_FORMAT_BYTES duration queries are forwarded all the way up to my_src, which returns the source file size. On the other hand, the convert query on stream->their_src expects the size of the decoded raw data. This patch uses parser->their_sink to convert the source file size to get a correct duration estimate.
Sorry for the complete silence. This patch set, and this patch in particular, has just been sitting there bothering me. I suspect that even if this patch set is correct it needs an audit of GStreamer elements to make sure it's always doing the right thing.
The basic problem is twofold: firstly, that GStreamer duration and convert queries are underspecified; and secondly, that the apparent actual behaviour is counterintuitive.
For example, I'd expect that a GST_FORMAT_TIME duration query should retrieve the actual running time of a stream (which might be different from the running time of a muxed file!), and that a GST_FORMAT_BYTES duration query should retrieve the total *output* size, in bytes, of a stream. But the default behaviour for any pad is to cheerfully ask the upstream filter, and GstBaseParse even explicitly does this *before* returning its own answers, so it'll end up returning the encoded size or even the whole (possibly muxed) file size. A similar problem occurs with GST_QUERY_CONVERT.
The ultimate effect is that the application can't predict who will respond to a duration or convert query, and thus they have no idea how to interpret the data they get back. In the case of DURATION + GST_FORMAT_TIME I suspect most applications get away with it, because usually the duration of a stream is (or is close enough to) the duration of a file. But GST_FORMAT_BYTES and any variation of CONVERT just seems completely broken...
Unfortunately, I'm not sure that downstream CONVERT queries are any better. The default behaviour is still to pass the query to a random src, and the first two demuxers I checked (avidemux and mpegpsdemux) both do that. I think both actually give us a correct duration in response to GST_QUERY_DURATION + GST_FORMAT_TIME, but it's not a good sign.
It may end up being best just to revert this path entirely, and rely on polling for GST_QUERY_DURATION + GST_FORMAT_TIME. The path was added by 613446d018. Looking back at mail history, I think I wrote it in response to [1], so it was actually just for dlls/quartz/tests/test.mp3.
It turns out I only got away with this because, in the case of mp3, there's no container, so the byte length of the encoded stream is the byte length of the file, mpg123 just happened to handle neither of these queries, and mpegaudioparse handled GST_QUERY_CONVERT in the intuitive way.
Unfortunately, unless I misread, I think there's currently no reliable way to get the duration of a CBR file from mpegaudioparse, except by relying on GST_QUERY_CONVERT.
AFAICS, mpegaudioparse should be able to get the duration of CBR files just fine since GstBaseParse provides a duration estimate every 50 frames until a subclass sets the actual duration. The only case where we have to fall back to GST_QUERY_CONVERT is a very short CBR file with GStreamer versions before 1.5.90. I don't know if these old versions are still relevant, though.
As long as we have patches 2/5 and 3/5 from this series, yeah. Thanks for pointing that out; I did miss that code. I don't think it's worth keeping 613446d018 around if this series is committed, just to work around an ancient GStreamer bug. I.e. I'd rather just leave out 5/5 and effectively revert that part altogether.
participants (2)
-
Anton Baskanov -
Zebediah Figura (she/her)