This patch set now has 24 test failures in wmvcore/tests/wmvcore.c: test_async_reader_streaming(). So it won't be merged now. But I'd like to have it reviewed to see if I was doing things in the right direction.
From: Ziqing Hui zhui@codeweavers.com
--- dlls/winegstreamer/quartz_parser.c | 4 +++- dlls/winegstreamer/wm_reader.c | 3 +++ dlls/wmvcore/tests/wmvcore.c | 14 +++++++++++++- 3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/dlls/winegstreamer/quartz_parser.c b/dlls/winegstreamer/quartz_parser.c index 0e5bdeca713..33f63e1988c 100644 --- a/dlls/winegstreamer/quartz_parser.c +++ b/dlls/winegstreamer/quartz_parser.c @@ -327,6 +327,9 @@ unsigned int wg_format_get_max_size(const struct wg_format *format) * but as long as every sample fits into our allocator, we're fine. */ return format->u.video_cinepak.width * format->u.video_cinepak.height * 3;
+ case WG_MAJOR_TYPE_VIDEO_WMV: + return format->u.video_wmv.width * format->u.video_wmv.height * 3; + case WG_MAJOR_TYPE_AUDIO: { unsigned int rate = format->u.audio.rate, channels = format->u.audio.channels; @@ -376,7 +379,6 @@ unsigned int wg_format_get_max_size(const struct wg_format *format) case WG_MAJOR_TYPE_AUDIO_MPEG4: case WG_MAJOR_TYPE_AUDIO_WMA: case WG_MAJOR_TYPE_VIDEO_H264: - case WG_MAJOR_TYPE_VIDEO_WMV: case WG_MAJOR_TYPE_VIDEO_INDEO: FIXME("Format %u not implemented!\n", format->major_type); return 0; diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index a6a5a81663e..02237933905 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -1826,6 +1826,9 @@ static HRESULT WINAPI reader_GetMaxStreamSampleSize(IWMSyncReader2 *iface, WORD
TRACE("reader %p, stream_number %u, size %p.\n", reader, stream_number, size);
+ if (!size) + return E_INVALIDARG; + EnterCriticalSection(&reader->cs);
if (!(stream = wm_reader_get_stream_by_stream_number(reader, stream_number))) diff --git a/dlls/wmvcore/tests/wmvcore.c b/dlls/wmvcore/tests/wmvcore.c index 6071ac25ad7..2022aed85a2 100644 --- a/dlls/wmvcore/tests/wmvcore.c +++ b/dlls/wmvcore/tests/wmvcore.c @@ -912,7 +912,7 @@ static void test_sync_reader_compressed(IWMSyncReader *reader) QWORD pts, duration; INSSBuffer *sample; WORD stream_number; - DWORD flags; + DWORD flags, size; HRESULT hr;
hr = IWMSyncReader_SetReadStreamSamples(reader, 0, TRUE); @@ -924,6 +924,18 @@ static void test_sync_reader_compressed(IWMSyncReader *reader) hr = IWMSyncReader_SetReadStreamSamples(reader, 3, TRUE); ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr);
+ hr = IWMSyncReader_GetMaxStreamSampleSize(reader, 0, &size); + ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr); + hr = IWMSyncReader_GetMaxStreamSampleSize(reader, 1, NULL); + ok(hr == E_INVALIDARG, "Got hr %#lx.\n", hr); + hr = IWMSyncReader_GetMaxStreamSampleSize(reader, 1, &size); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + ok(size == 64 * 48 * 3, "Got unexpected size %lu.\n", size); + hr = IWMSyncReader_GetMaxStreamSampleSize(reader, 2, &size); + ok(hr == S_OK, "Got hr %#lx.\n", hr); + todo_wine + ok(size == 743, "Got unexpected size %lu.\n", size); + hr = IWMSyncReader_SetRange(reader, 0, 0); ok(hr == S_OK, "Got hr %#lx.\n", hr);
From: Ziqing Hui zhui@codeweavers.com
--- dlls/winegstreamer/wg_format.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_format.c b/dlls/winegstreamer/wg_format.c index bd0ebf892c1..0110d4980b4 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -725,7 +725,6 @@ bool wg_format_compare(const struct wg_format *a, const struct wg_format *b) case WG_MAJOR_TYPE_AUDIO_MPEG4: case WG_MAJOR_TYPE_AUDIO_WMA: case WG_MAJOR_TYPE_VIDEO_H264: - case WG_MAJOR_TYPE_VIDEO_WMV: case WG_MAJOR_TYPE_VIDEO_INDEO: GST_FIXME("Format %u not implemented!", a->major_type); /* fallthrough */ @@ -748,6 +747,12 @@ bool wg_format_compare(const struct wg_format *a, const struct wg_format *b) /* Do not compare FPS. */ return a->u.video_cinepak.width == b->u.video_cinepak.width && a->u.video_cinepak.height == b->u.video_cinepak.height; + + case WG_MAJOR_TYPE_VIDEO_WMV: + /* Do not compare FPS. */ + return a->u.video_wmv.format == b->u.video_wmv.format + && a->u.video_wmv.width == b->u.video_wmv.width + && a->u.video_wmv.height == b->u.video_wmv.height; }
assert(0);
From: Ziqing Hui zhui@codeweavers.com
--- dlls/winegstreamer/gst_private.h | 1 + dlls/winegstreamer/main.c | 13 +++ dlls/winegstreamer/unixlib.h | 7 ++ dlls/winegstreamer/wg_parser.c | 194 +++++++++++++++++++++---------- dlls/winegstreamer/wm_reader.c | 12 ++ 5 files changed, 166 insertions(+), 61 deletions(-)
diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index d397ab21ca0..ebbbf4e6510 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -100,6 +100,7 @@ char *wg_parser_stream_get_tag(struct wg_parser_stream *stream, enum wg_parser_t /* start_pos and stop_pos are in 100-nanosecond units. */ void wg_parser_stream_seek(struct wg_parser_stream *stream, double rate, uint64_t start_pos, uint64_t stop_pos, DWORD start_flags, DWORD stop_flags); +bool wg_parser_stream_set_output_compressed(struct wg_parser_stream *stream, bool compressed);
struct wg_transform *wg_transform_create(const struct wg_format *input_format, const struct wg_format *output_format, const struct wg_transform_attrs *attrs); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index 88179feeb56..e93f226ef02 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -317,6 +317,19 @@ char *wg_parser_stream_get_tag(struct wg_parser_stream *stream, enum wg_parser_t return buffer; }
+bool wg_parser_stream_set_output_compressed(struct wg_parser_stream *stream, bool compressed) +{ + struct wg_parser_stream_set_output_compressed_params params = + { + .stream = stream, + .compressed = compressed, + }; + + TRACE("stream %p, compressed %d.\n", stream, compressed); + + return !WINE_UNIX_CALL(unix_wg_parser_stream_set_output_compressed, ¶ms); +} + void wg_parser_stream_seek(struct wg_parser_stream *stream, double rate, uint64_t start_pos, uint64_t stop_pos, DWORD start_flags, DWORD stop_flags) { diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index a9892bf9d4f..ee4cbec76c1 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -282,6 +282,12 @@ struct wg_parser_stream_get_duration_params UINT64 duration; };
+struct wg_parser_stream_set_output_compressed_params +{ + struct wg_parser_stream *stream; + bool compressed; +}; + enum wg_parser_tag { WG_PARSER_TAG_LANGUAGE, @@ -376,6 +382,7 @@ enum unix_funcs unix_wg_parser_stream_get_duration, unix_wg_parser_stream_get_tag, unix_wg_parser_stream_seek, + unix_wg_parser_stream_set_output_compressed,
unix_wg_transform_create, unix_wg_transform_destroy, diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index cb2901f484d..2291768db74 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -110,10 +110,10 @@ struct wg_parser_stream struct wg_format preferred_format, current_format, codec_format;
pthread_cond_t event_cond, event_empty_cond; - GstBuffer *buffer; - GstMapInfo map_info; + GstBuffer *buffer, *buffer_compressed; + GstMapInfo map_info, map_info_compressed;
- bool flushing, eos, enabled, has_caps, has_tags, has_buffer, no_more_pads; + bool flushing, eos, enabled, has_caps, has_tags, has_buffer, no_more_pads, output_compressed;
uint64_t duration; gchar *tags[WG_PARSER_TAG_COUNT]; @@ -126,6 +126,73 @@ static bool format_is_compressed(struct wg_format *format) && format->major_type != WG_MAJOR_TYPE_AUDIO; }
+static bool stream_buffer_available(struct wg_parser_stream *stream) +{ + return format_is_compressed(&stream->codec_format) ? + stream->buffer && stream->buffer_compressed : + !!stream->buffer; +} + +static GstFlowReturn stream_set_buffer(struct wg_parser_stream *stream, GstBuffer *buffer, bool compressed) +{ + struct wg_parser *parser = stream->parser; + GstMapInfo *map_info; + + pthread_mutex_lock(&parser->mutex); + + if (!compressed && !stream->has_buffer) + { + stream->has_buffer = true; + pthread_cond_signal(&parser->init_cond); + } + + /* Allow this buffer to be flushed by GStreamer. We are effectively + * implementing a queue object here. */ + + while (stream->enabled && !stream->flushing && (compressed ? stream->buffer_compressed : stream->buffer)) + pthread_cond_wait(&stream->event_empty_cond, &parser->mutex); + + if (!stream->enabled) + { + pthread_mutex_unlock(&parser->mutex); + gst_buffer_unref(buffer); + return GST_FLOW_OK; + } + + if (stream->flushing) + { + pthread_mutex_unlock(&parser->mutex); + GST_DEBUG("Stream is flushing; discarding buffer."); + gst_buffer_unref(buffer); + return GST_FLOW_FLUSHING; + } + + map_info = compressed ? &stream->map_info_compressed : &stream->map_info; + if (!gst_buffer_map(buffer, map_info, GST_MAP_READ)) + { + pthread_mutex_unlock(&parser->mutex); + GST_ERROR("Failed to map buffer.\n"); + gst_buffer_unref(buffer); + return GST_FLOW_ERROR; + } + + if (compressed) + stream->buffer_compressed = buffer; + else + stream->buffer = buffer; + + pthread_mutex_unlock(&parser->mutex); + pthread_cond_signal(&stream->event_cond); + + /* The chain callback is given a reference to the buffer. Transfer that + * reference to the stream object, which will release it in + * wg_parser_stream_release_buffer(). */ + + GST_LOG("Buffer queued."); + + return GST_FLOW_OK; +} + static NTSTATUS wg_parser_get_stream_count(void *args) { struct wg_parser_get_stream_count_params *params = args; @@ -266,15 +333,21 @@ static NTSTATUS wg_parser_stream_disable(void *args)
static GstBuffer *wait_parser_stream_buffer(struct wg_parser *parser, struct wg_parser_stream *stream) { - GstBuffer *buffer = NULL; - /* Note that we can both have a buffer and stream->eos, in which case we * must return the buffer. */
- while (stream->enabled && !(buffer = stream->buffer) && !stream->eos) + while (stream->enabled && !stream_buffer_available(stream) && !stream->eos) pthread_cond_wait(&stream->event_cond, &parser->mutex);
- return buffer; + if (stream->buffer_compressed) + { + if (!GST_BUFFER_PTS_IS_VALID(stream->buffer_compressed) && GST_BUFFER_PTS_IS_VALID(stream->buffer)) + GST_BUFFER_PTS(stream->buffer_compressed) = GST_BUFFER_PTS(stream->buffer); + if (!GST_BUFFER_DURATION_IS_VALID(stream->buffer_compressed) && GST_BUFFER_DURATION_IS_VALID(stream->buffer)) + GST_BUFFER_DURATION(stream->buffer_compressed) = GST_BUFFER_DURATION(stream->buffer); + } + + return stream->output_compressed ? stream->buffer_compressed : stream->buffer; }
static NTSTATUS wg_parser_stream_get_buffer(void *args) @@ -356,18 +429,20 @@ static NTSTATUS wg_parser_stream_copy_buffer(void *args) struct wg_parser *parser = stream->parser; uint32_t offset = params->offset; uint32_t size = params->size; + GstMapInfo *map_info;
pthread_mutex_lock(&parser->mutex);
- if (!stream->buffer) + if (!stream_buffer_available(stream)) { pthread_mutex_unlock(&parser->mutex); return VFW_E_WRONG_STATE; }
- assert(offset < stream->map_info.size); - assert(offset + size <= stream->map_info.size); - memcpy(params->data, stream->map_info.data + offset, size); + map_info = stream->output_compressed ? &stream->map_info_compressed : &stream->map_info; + assert(offset < map_info->size); + assert(offset + size <= map_info->size); + memcpy(params->data, map_info->data + offset, size);
pthread_mutex_unlock(&parser->mutex); return S_OK; @@ -381,11 +456,17 @@ static NTSTATUS wg_parser_stream_release_buffer(void *args) pthread_mutex_lock(&parser->mutex);
assert(stream->buffer); - gst_buffer_unmap(stream->buffer, &stream->map_info); gst_buffer_unref(stream->buffer); stream->buffer = NULL;
+ if (stream->buffer_compressed) + { + gst_buffer_unmap(stream->buffer_compressed, &stream->map_info_compressed); + gst_buffer_unref(stream->buffer_compressed); + stream->buffer_compressed = NULL; + } + pthread_mutex_unlock(&parser->mutex); pthread_cond_signal(&stream->event_empty_cond);
@@ -473,6 +554,19 @@ static NTSTATUS wg_parser_stream_notify_qos(void *args) return S_OK; }
+static NTSTATUS wg_parser_stream_set_output_compressed(void *args) +{ + const struct wg_parser_stream_set_output_compressed_params *params = args; + struct wg_parser_stream *stream = params->stream; + struct wg_parser *parser = stream->parser; + + pthread_mutex_lock(&parser->mutex); + stream->output_compressed = params->compressed; + pthread_mutex_unlock(&parser->mutex); + + return S_OK; +} + static bool parser_no_more_pads(struct wg_parser *parser) { unsigned int i; @@ -586,6 +680,12 @@ static gboolean sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event) gst_buffer_unref(stream->buffer); stream->buffer = NULL; } + if (stream->buffer_compressed) + { + gst_buffer_unmap(stream->buffer_compressed, &stream->map_info_compressed); + gst_buffer_unref(stream->buffer_compressed); + stream->buffer_compressed = NULL; + } }
pthread_mutex_unlock(&parser->mutex); @@ -640,58 +740,10 @@ static gboolean sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event) static GstFlowReturn sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) { struct wg_parser_stream *stream = gst_pad_get_element_private(pad); - struct wg_parser *parser = stream->parser;
GST_LOG("stream %p, buffer %p.", stream, buffer);
- pthread_mutex_lock(&parser->mutex); - - if (!stream->has_buffer) - { - stream->has_buffer = true; - pthread_cond_signal(&parser->init_cond); - } - - /* Allow this buffer to be flushed by GStreamer. We are effectively - * implementing a queue object here. */ - - while (stream->enabled && !stream->flushing && stream->buffer) - pthread_cond_wait(&stream->event_empty_cond, &parser->mutex); - - if (!stream->enabled) - { - pthread_mutex_unlock(&parser->mutex); - gst_buffer_unref(buffer); - return GST_FLOW_OK; - } - - if (stream->flushing) - { - pthread_mutex_unlock(&parser->mutex); - GST_DEBUG("Stream is flushing; discarding buffer."); - gst_buffer_unref(buffer); - return GST_FLOW_FLUSHING; - } - - if (!gst_buffer_map(buffer, &stream->map_info, GST_MAP_READ)) - { - pthread_mutex_unlock(&parser->mutex); - GST_ERROR("Failed to map buffer.\n"); - gst_buffer_unref(buffer); - return GST_FLOW_ERROR; - } - - stream->buffer = buffer; - - pthread_mutex_unlock(&parser->mutex); - pthread_cond_signal(&stream->event_cond); - - /* The chain callback is given a reference to the buffer. Transfer that - * reference to the stream object, which will release it in - * wg_parser_stream_release_buffer(). */ - - GST_LOG("Buffer queued."); - return GST_FLOW_OK; + return stream_set_buffer(stream, buffer, FALSE); }
static gboolean sink_query_cb(GstPad *pad, GstObject *parent, GstQuery *query) @@ -775,6 +827,17 @@ static gboolean sink_query_cb(GstPad *pad, GstObject *parent, GstQuery *query) } }
+static GstPadProbeReturn compressed_data_buffer_cb(GstPad *pad, GstPadProbeInfo *info, gpointer user_data) +{ + GstBuffer *buffer = GST_PAD_PROBE_INFO_BUFFER(info); + struct wg_parser_stream *stream = user_data; + + gst_buffer_ref(buffer); + stream_set_buffer(stream, buffer, TRUE); + + return GST_PAD_PROBE_OK; +} + static struct wg_parser_stream *create_stream(struct wg_parser *parser) { struct wg_parser_stream *stream, **new_array; @@ -819,6 +882,12 @@ static void free_stream(struct wg_parser_stream *stream) gst_buffer_unref(stream->buffer); stream->buffer = NULL; } + if (stream->buffer_compressed) + { + gst_buffer_unmap(stream->buffer_compressed, &stream->map_info_compressed); + gst_buffer_unref(stream->buffer_compressed); + stream->buffer_compressed = NULL; + }
pthread_cond_destroy(&stream->event_cond); pthread_cond_destroy(&stream->event_empty_cond); @@ -969,6 +1038,8 @@ static void pad_added_cb(GstElement *element, GstPad *pad, gpointer user) /* For compressed stream, create an extra decodebin to decode it. */ if (format_is_compressed(&stream->codec_format)) { + gst_pad_add_probe(pad, GST_PAD_PROBE_TYPE_BUFFER, compressed_data_buffer_cb, stream, NULL); + if (!stream_decodebin_create(stream)) { GST_ERROR("Failed to create decodebin for stream %u.", stream->number); @@ -1931,6 +2002,7 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_parser_stream_get_duration), X(wg_parser_stream_get_tag), X(wg_parser_stream_seek), + X(wg_parser_stream_set_output_compressed),
X(wg_transform_create), X(wg_transform_destroy), diff --git a/dlls/winegstreamer/wm_reader.c b/dlls/winegstreamer/wm_reader.c index 02237933905..3d279b96d0b 100644 --- a/dlls/winegstreamer/wm_reader.c +++ b/dlls/winegstreamer/wm_reader.c @@ -2338,6 +2338,18 @@ static HRESULT WINAPI reader_SetReadStreamSamples(IWMSyncReader2 *iface, WORD st return E_INVALIDARG; }
+ if (stream->read_compressed == compressed) + { + LeaveCriticalSection(&reader->cs); + return S_OK; + } + + if (!wg_parser_stream_set_output_compressed(stream->wg_stream, compressed)) + { + LeaveCriticalSection(&reader->cs); + return E_FAIL; + } + stream->read_compressed = compressed;
LeaveCriticalSection(&reader->cs);
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134297
Your paranoid android.
=== debian11 (32 bit report) ===
wmp: media: Timeout
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit ar:MA report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit de report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit fr report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit he:IL report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit hi:IN report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit ja:JP report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11 (32 bit zh:CN report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11b (32 bit WoW report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
=== debian11b (64 bit WoW report) ===
wmvcore: wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000. wmvcore.c:2685: Test failed: Got pts 0. wmvcore.c:2071: Test failed: got time 0 wmvcore.c:2757: Test failed: Got pts 13260000.
I think the idea here was to detach the stream decodebin if we're outputting compressed samples, and use the same actual sink pad for both compressed and uncompressed cases? Using a probe means that the samples will get fed into the decodebin anyway, which ends up just wasting CPU on decoding.
On Fri Jun 30 04:01:30 2023 +0000, Zebediah Figura wrote:
I think the idea here was to detach the stream decodebin if we're outputting compressed samples, and use the same actual sink pad for both compressed and uncompressed cases? Using a probe means that the samples will get fed into the decodebin anyway, which ends up just wasting CPU on decoding.
I tried the unlinking/linking stream decodebin approach. However, it's really complex to implement that, because changing pipeline dynamically needs to consider a lot of stuff (eg. renegotiate caps, seek to restart pipeline, flush previous buffers). Also, buffer loss happens because of the pipeline changes.
Instead, adding a probe is quite simple and easy to implement.
Using a probe means that the samples will get fed into the decodebin anyway, which ends up just wasting CPU on decoding.
We can prevent the buffer from going into stream decodebin by return GST_PAD_PROBE_DROP or GST_PAD_PROBE_HANDLED in probe.
On Fri Jun 30 04:01:38 2023 +0000, Ziqing Hui wrote:
I tried the unlinking/linking stream decodebin approach. However, it's really complex to implement that, because changing pipeline dynamically needs to consider a lot of stuff (eg. renegotiate caps, seek to restart pipeline, flush previous buffers). Also, buffer loss happens because of the pipeline changes. Instead, adding a probe is quite simple and easy to implement.
Using a probe means that the samples will get fed into the decodebin
anyway, which ends up just wasting CPU on decoding. We can prevent the buffer from going into stream decodebin by return GST_PAD_PROBE_DROP or GST_PAD_PROBE_HANDLED in probe.
Consider the following code:
``` foo() { [...] reader_OpenStream(); reader_SetReadStreamSamples(TRUE); reader_GetNextSample(reader); [...] } ```
After OpenStream, the decoding thread in our pipeline is already started. So before SetReadStreamSamples, there is posibility that a uncompressed frame, which is the first frame, is already in our stream->buffer. If we detach the stream decodebin in SetReadStreamSamples, the compressed buffer we grab in GetNextSample is the second frame, unless we seek to where we want to be.
Adding a probe to grab compressed buffer will avoid this problem, because we always record the compressed buffer even if the stream is not set to output compressed. We don't have buffer loss when changing from uncompressed state to compressed state.
I tried the unlinking/linking stream decodebin approach. However, it's really complex to implement that, because changing pipeline dynamically needs to consider a lot of stuff (eg. renegotiate caps, seek to restart pipeline, flush previous buffers). Also, buffer loss happens because of the pipeline changes.
Instead, adding a probe is quite simple and easy to implement.
Using a probe means that the samples will get fed into the decodebin anyway, which ends up just wasting CPU on decoding.
We can prevent the buffer from going into stream decodebin by return GST_PAD_PROBE_DROP or GST_PAD_PROBE_HANDLED in probe, which I think is identical to "detach".
Hmm, okay, interesting. I had been assuming that all of this was unavoidable, but on reflection it does seem that the probe is surprisingly simple and should work.
After OpenStream, the decoding thread in our pipeline is already started. So before SetReadStreamSamples, there is posibility that a uncompressed frame, which is the first frame, is already in our stream->buffer. If we detach the stream decodebin in SetReadStreamSamples, the compressed buffer we grab in GetNextSample is the second frame, unless we seek to where we want to be.
So this is already a problem we have when changing the output format (between two uncompressed formats). At that point I figured that the problem is intractable, and that the sensible thing to do would just be to seek back to the beginning: https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/winegstreamer/wm_reader.c#l2244
The idea of grabbing a reference to the compressed sample is interesting, and I feel like it *almost* solves the problem. The only issue is that the decoding pipeline isn't forbidden to have a queue in it somewhere, which would violate the assumptions we make here... but in practice it's not particularly likely to, so it could work anyway.
Of course, the other thing to consider is that switching from compressed to uncompressed mid-stream seems even less likely than switching formats, and if we decide we don't want to care about that either, we can probably simplify some logic—like not using separate fields for the compressed and uncompressed streams. But on the other hand, the patch as-is doesn't feel complex enough to bother me, at least now that I understand the idea.
One thing we may consider doing, that would probably simplify things a bit, is not mapping samples immediately. I think that's a holdover from when we would return the sample address directly to the client, but we're not doing that anymore—we could just map and unmap it in wg_parser_stream_copy_buffer(). As far as this patch is concerned that would remove the need to map the compressed and uncompressed samples separately, and remove a couple of ternaries.
Sorry for the delay in reviewing. This looks good just from reading over it, but it needs a rebase now that the wow64 thunks are in.
On Thu Jul 13 02:09:12 2023 +0000, Zebediah Figura wrote:
Sorry for the delay in reviewing. This looks good just from reading over it, but it needs a rebase now that the wow64 thunks are in.
I'm working implementing winegstreamer muxer recently. I'll update this patchset after the initial stuff for muxer work is done.
Fwiw like I said elsewhere, I don't think that hooking the decoderbin decoders input and stealing compressed buffers from the pipeline is the right way to do this, and I think it adds a lot of complexity.
I'm not going to block this, for various reasons, if you want to do it that way in the wmp backend, but I think it should be done differently.
Fwiw like I said elsewhere, I don't think that hooking the decoderbin decoders input and stealing compressed buffers from the pipeline is the right way to do this, and I think it adds a lot of complexity.
I would like to be wary of situations where a contributor, having written code, to find it simple and straightforward (at least in the moment) while a reviewer finds it anything but. I did not write this code, but I did (IIRC) propose the general design, and so if someone else finds it complex or unintuitive, I'd like to take that into account.
Hence, while I know you said you've gone over the problems with this design in the past, but I'm afraid that so much has been said that I cannot recall those conversations, or at least, I cannot recall any specific problems that were raised, so I'd appreciate if you'd repeat here. What is it about this approach is it that seems wrong to you?
For what it's worth, here's an attempt at detailing the thought process that leads me to take stock in this design:
Some application depends on creating a demuxer, one that is currently backed by wg_parser. Instead of receiving fully uncompressed samples, it needs to recieve compressed samples, but in all other respects, the process of demuxing remains the same. Hence all we need to do is modify the current code so that we don't decode all the way. decodebin makes it very easy to do this, by simply stopping the process of decoding before it gets to the end.
One part of this problem statement is that "compressed samples" is vague. What do we count as a compressed format? In practice, I think this needs to be a manually written whitelist of formats. There are two reasons:
- If we suddenly *actually* output compressed samples for all types, an application which does ultimately want to autoplug to raw might be stymied by the lack of a decoder component in Wine. This way makes it very easy to stop on new formats one at a time, when we are relatively more sure that we can handle them. This point is less salient if the application has to currently manually enable the feature via a wmvcore API. On the other hand, I'm looking forward to using this feature in other places; e.g. I know that some mfplat applications need this.
- Not all formats exist on Windows. We want to support things like media players with arbitrary formats—we want users to be able to play their ogg vorbis files, and so on. We could add these formats, but that would be a lot of work, and some of them may be nontrivial. If we just keep decodebin instead, that way we get everything, right away, for free.
With that said, some questions that occurred to me while thinking about this:
* **Do we really want to use decodebin for this? Should we write our own autoplugging code?** I think using decodebin is the right choice. We need pretty much all of the decodebin features we're currently using, and rewriting them would be a lot of work for no benefit I can see.
Using decodebin currently comprises two steps: start the autoplugging, and provide a callback that determines whether we should stop at any given point. The approach that Ziqing takes is a bit more complex, because the autoplugging needs to be conditional: start the autoplugging, stop (always), then append another decodebin and autoplug from there. *Not* using decodebin means that we have to rewrite not just all of the autoplugging logic, but other decodebin features we need, such as detecting parsers and multiqueuing.
Note that we can't just look for a demuxer element and have done, for one, and possibly two reasons. The first reason is described above: we don't want to stop at *every* compressed format. The second reason is that we might need to append an extra element for some formats anyway, although I'm not sure of this. I'm thinking of elements like h264parse, which do some preprocessing on the compressed formats but don't actually decompress them. I don't know if that element is necessary, but even if not, something like it might be.
* **Should we use parsebin instead? It seems made for this purpose.** Problem is, parsebin is a relatively recent feature. More to the point, we can't "just" use parsebin everywhere, for the same reasons.
* **Should we actually use wg_parser for this, though? Adding another feature to wg_parser increases the complexity, and this does touch a lot of core code.** On the other hand, we do need the rest of wg_parser, so duplicating all that to another winegstreamer backend would not really help anything.
Some of the parts of Ziqing's patches that you mentioned also left a distaste in my mouth in I think the same way. In particular:
- the use of a probe never seems like the cleanest approach. On the other hand, probes are pretty clearly documented, so we know this works, and I don't have a better objection than "it feels ugly". That said... while recreating the pipeline *within* wg_parser gets complicated, as Ziqing says, I still wouldn't object to rewriting this so that we just destroy and recreate the wg_parser?
- the extra code to try to handle toggling mid-stream is added complexity that will probably never actually be required. I'd prefer to just get rid of it, honestly, but I find it difficult to say as much in a review, when it is a feature that exists, and I have a hard time saying I hate it.
This merge request was closed by Ziqing Hui.
-
Marvin -
Rémi Bernon (＠rbernon) -
Zebediah Figura (＠zfigura)
-
Ziqing Hui
-
Ziqing Hui (＠zhui)