[PATCH v2 0/7] MR2719: winegstreamer: Cleanup media source streams and add synchronization.

List overview All Threads

newer

older

Re: [PATCH v2 0/4] MR174:...

Rémi Bernon (＠rbernon)

1 May 2023 1 May '23

9:11 a.m.

Mostly a refresh of https://gitlab.winehq.org/wine/wine/-/merge_requests/1278, with a bit of cleanup. More to come.

-- v2: winegstreamer: Remove unnecessary media source stream states. winegstreamer: Synchronize access to the media source from callbacks. winegstreamer: Synchronize concurrent access to the media stream. winegstreamer: Synchronize concurrent access to the media source. winegstreamer: Only break cyclic references in IMFMediaSource_Shutdown. winegstreamer: Keep a IMFMediaSource pointer in the media stream. winegstreamer: Query the wg_parser stream in media_stream_create.

https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Show replies by date

Rémi Bernon

1 May 1 May

9:11 a.m.

New subject: [PATCH v2 1/7] winegstreamer: Query the wg_parser stream in media_stream_create.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/winegstreamer/media_source.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index e9db8dcc0a2..cbd78b17118 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -821,13 +821,16 @@ static const IMFMediaStreamVtbl media_stream_vtbl = media_stream_RequestSample };

-static HRESULT new_media_stream(struct media_source *source, - struct wg_parser_stream *wg_stream, DWORD stream_id, struct media_stream **out_stream) +static HRESULT media_stream_create(struct media_source *source, DWORD id, + struct media_stream **out) { - struct media_stream *object = calloc(1, sizeof(*object)); + struct media_stream *object; HRESULT hr;

- TRACE("source %p, wg_stream %p, stream_id %lu.\n", source, wg_stream, stream_id); + TRACE("source %p, id %lu.\n", source, id); + + if (!(object = calloc(1, sizeof(*object)))) + return E_OUTOFMEMORY;

object->IMFMediaStream_iface.lpVtbl = &media_stream_vtbl; object->ref = 1; @@ -840,16 +843,15 @@ static HRESULT new_media_stream(struct media_source *source,

IMFMediaSource_AddRef(&source->IMFMediaSource_iface); object->parent_source = source; - object->stream_id = stream_id; + object->stream_id = id;

object->state = STREAM_INACTIVE; object->eos = FALSE; - object->wg_stream = wg_stream; + object->wg_stream = wg_parser_get_stream(source->wg_parser, id);

TRACE("Created stream object %p.\n", object);

- *out_stream = object; - + *out = object; return S_OK; }

@@ -1473,7 +1475,7 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_

for (i = 0; i < stream_count; ++i) { - if (FAILED(hr = new_media_stream(object, wg_parser_get_stream(parser, i), i, &object->streams[i]))) + if (FAILED(hr = media_stream_create(object, i, &object->streams[i]))) goto fail;

if (FAILED(hr = media_stream_init_desc(object->streams[i])))

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Rémi Bernon

9:11 a.m.

New subject: [PATCH v2 2/7] winegstreamer: Keep a IMFMediaSource pointer in the media stream.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/winegstreamer/media_source.c | 42 +++++++++++++++++-------------- 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index cbd78b17118..c9fdbdcf703 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -31,7 +31,8 @@ struct media_stream { IMFMediaStream IMFMediaStream_iface; LONG ref; - struct media_source *parent_source; + + IMFMediaSource *media_source; IMFMediaEventQueue *event_queue; IMFStreamDescriptor *descriptor;

@@ -288,6 +289,7 @@ static BOOL enqueue_token(struct media_stream *stream, IUnknown *token)

static void flush_token_queue(struct media_stream *stream, BOOL send) { + struct media_source *source = impl_from_IMFMediaSource(stream->media_source); LONG i;

for (i = 0; i < stream->token_queue_count; i++) @@ -301,8 +303,8 @@ static void flush_token_queue(struct media_stream *stream, BOOL send) command->u.request_sample.stream = stream; command->u.request_sample.token = stream->token_queue[i];

- hr = MFPutWorkItem(stream->parent_source->async_commands_queue, - &stream->parent_source->async_commands_callback, &command->IUnknown_iface); + hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, + &command->IUnknown_iface); } if (FAILED(hr)) WARN("Could not enqueue sample request, hr %#lx\n", hr); @@ -528,7 +530,7 @@ out:

static void wait_on_sample(struct media_stream *stream, IUnknown *token) { - struct media_source *source = stream->parent_source; + struct media_source *source = impl_from_IMFMediaSource(stream->media_source); PROPVARIANT empty_var = {.vt = VT_EMPTY}; struct wg_parser_buffer buffer;

@@ -542,7 +544,7 @@ static void wait_on_sample(struct media_stream *stream, IUnknown *token) { stream->eos = TRUE; IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEEndOfStream, &GUID_NULL, S_OK, &empty_var); - dispatch_end_of_presentation(stream->parent_source); + dispatch_end_of_presentation(source); } }

@@ -741,17 +743,18 @@ static HRESULT WINAPI media_stream_QueueEvent(IMFMediaStream *iface, MediaEventT return IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, event_type, ext_type, hr, value); }

-static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMediaSource **source) +static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMediaSource **out) { struct media_stream *stream = impl_from_IMFMediaStream(iface); + struct media_source *source = impl_from_IMFMediaSource(stream->media_source);

- TRACE("%p, %p.\n", iface, source); + TRACE("%p, %p.\n", iface, out);

if (stream->state == STREAM_SHUTDOWN) return MF_E_SHUTDOWN;

- IMFMediaSource_AddRef(&stream->parent_source->IMFMediaSource_iface); - *source = &stream->parent_source->IMFMediaSource_iface; + IMFMediaSource_AddRef(&source->IMFMediaSource_iface); + *out = &source->IMFMediaSource_iface;

return S_OK; } @@ -774,6 +777,7 @@ static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IM static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) { struct media_stream *stream = impl_from_IMFMediaStream(iface); + struct media_source *source = impl_from_IMFMediaSource(stream->media_source); struct source_async_command *command; HRESULT hr;

@@ -800,8 +804,7 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown IUnknown_AddRef(token); command->u.request_sample.token = token;

return hr; @@ -821,9 +824,10 @@ static const IMFMediaStreamVtbl media_stream_vtbl = media_stream_RequestSample };

-static HRESULT media_stream_create(struct media_source *source, DWORD id, +static HRESULT media_stream_create(IMFMediaSource *source, DWORD id, struct media_stream **out) { + struct wg_parser *wg_parser = impl_from_IMFMediaSource(source)->wg_parser; struct media_stream *object; HRESULT hr;

@@ -841,13 +845,13 @@ static HRESULT media_stream_create(struct media_source *source, DWORD id, return hr; }

- IMFMediaSource_AddRef(&source->IMFMediaSource_iface); - object->parent_source = source; + IMFMediaSource_AddRef(source); + object->media_source = source; object->stream_id = id;

object->state = STREAM_INACTIVE; object->eos = FALSE; - object->wg_stream = wg_parser_get_stream(source->wg_parser, id); + object->wg_stream = wg_parser_get_stream(wg_parser, id);

TRACE("Created stream object %p.\n", object);

@@ -1368,7 +1372,7 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)

IMFMediaEventQueue_Shutdown(stream->event_queue); IMFStreamDescriptor_Release(stream->descriptor); - IMFMediaSource_Release(&stream->parent_source->IMFMediaSource_iface); + IMFMediaSource_Release(stream->media_source);

IMFMediaStream_Release(&stream->IMFMediaStream_iface); } @@ -1475,13 +1479,13 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_

for (i = 0; i < stream_count; ++i) { - if (FAILED(hr = media_stream_create(object, i, &object->streams[i]))) + if (FAILED(hr = media_stream_create(&object->IMFMediaSource_iface, i, &object->streams[i]))) goto fail;

if (FAILED(hr = media_stream_init_desc(object->streams[i]))) { ERR("Failed to finish initialization of media stream %p, hr %#lx.\n", object->streams[i], hr); - IMFMediaSource_Release(&object->streams[i]->parent_source->IMFMediaSource_iface); + IMFMediaSource_Release(object->streams[i]->media_source); IMFMediaEventQueue_Release(object->streams[i]->event_queue); free(object->streams[i]); goto fail; @@ -1567,7 +1571,7 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_

IMFMediaEventQueue_Release(stream->event_queue); IMFStreamDescriptor_Release(stream->descriptor); - IMFMediaSource_Release(&stream->parent_source->IMFMediaSource_iface); + IMFMediaSource_Release(stream->media_source);

free(stream); }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Rémi Bernon

9:11 a.m.

New subject: [PATCH v2 3/7] winegstreamer: Only break cyclic references in IMFMediaSource_Shutdown.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/winegstreamer/media_source.c | 38 ++++++++++++------------------- 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index c9fdbdcf703..5221668b10c 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -697,8 +697,9 @@ static ULONG WINAPI media_stream_Release(IMFMediaStream *iface)

if (!ref) { - if (stream->event_queue) - IMFMediaEventQueue_Release(stream->event_queue); + IMFMediaSource_Release(stream->media_source); + IMFStreamDescriptor_Release(stream->descriptor); + IMFMediaEventQueue_Release(stream->event_queue); flush_token_queue(stream, FALSE); free(stream); } @@ -1205,8 +1206,11 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface)

if (!ref) { - IMFMediaSource_Shutdown(&source->IMFMediaSource_iface); + IMFMediaSource_Shutdown(iface); + IMFPresentationDescriptor_Release(source->pres_desc); IMFMediaEventQueue_Release(source->event_queue); + IMFByteStream_Release(source->byte_stream); + wg_parser_destroy(source->wg_parser); free(source); }

@@ -1344,7 +1348,6 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface) static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_source *source = impl_from_IMFMediaSource(iface); - unsigned int i;

TRACE("%p.\n", iface);

@@ -1359,26 +1362,16 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) WaitForSingleObject(source->read_thread, INFINITE); CloseHandle(source->read_thread);

- IMFPresentationDescriptor_Release(source->pres_desc); IMFMediaEventQueue_Shutdown(source->event_queue); IMFByteStream_Close(source->byte_stream); - IMFByteStream_Release(source->byte_stream);

- for (i = 0; i < source->stream_count; i++) + while (source->stream_count--) { - struct media_stream *stream = source->streams[i]; - + struct media_stream *stream = source->streams[source->stream_count]; stream->state = STREAM_SHUTDOWN; - IMFMediaEventQueue_Shutdown(stream->event_queue); - IMFStreamDescriptor_Release(stream->descriptor); - IMFMediaSource_Release(stream->media_source); - IMFMediaStream_Release(&stream->IMFMediaStream_iface); } - - wg_parser_destroy(source->wg_parser); - free(source->streams);

MFUnlockWorkQueue(source->async_commands_queue); @@ -1565,17 +1558,14 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_ IMFStreamDescriptor_Release(descriptors[i]); free(descriptors); } - for (i = 0; i < object->stream_count; i++) - { - struct media_stream *stream = object->streams[i];

- IMFMediaEventQueue_Release(stream->event_queue); - IMFStreamDescriptor_Release(stream->descriptor); - IMFMediaSource_Release(stream->media_source); - - free(stream); + while (object->streams && object->stream_count--) + { + struct media_stream *stream = object->streams[object->stream_count]; + IMFMediaStream_Release(&stream->IMFMediaStream_iface); } free(object->streams); + if (stream_count != UINT_MAX) wg_parser_disconnect(object->wg_parser); if (object->read_thread)

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Giovanni Mascellani

9:11 a.m.

New subject: [PATCH v2 4/7] winegstreamer: Synchronize concurrent access to the media source.

From: Giovanni Mascellani gmascellani@codeweavers.com

--- dlls/winegstreamer/media_source.c | 76 +++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 20 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index 5221668b10c..aadb4d8a42c 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -93,6 +93,8 @@ struct media_source IMFMediaEventQueue *event_queue; IMFByteStream *byte_stream;

+ CRITICAL_SECTION cs; + struct wg_parser *wg_parser;

struct media_stream **streams; @@ -1132,7 +1134,9 @@ static HRESULT WINAPI media_source_rate_control_SetRate(IMFRateControl *iface, B if (FAILED(hr = IMFRateSupport_IsRateSupported(&source->IMFRateSupport_iface, thin, rate, NULL))) return hr;

+ EnterCriticalSection(&source->cs); source->rate = rate; + LeaveCriticalSection(&source->cs);

return IMFMediaEventQueue_QueueEventParamVar(source->event_queue, MESourceRateChanged, &GUID_NULL, S_OK, NULL); } @@ -1146,7 +1150,9 @@ static HRESULT WINAPI media_source_rate_control_GetRate(IMFRateControl *iface, B if (thin) *thin = FALSE;

+ EnterCriticalSection(&source->cs); *rate = source->rate; + LeaveCriticalSection(&source->cs);

return S_OK; } @@ -1211,6 +1217,8 @@ static ULONG WINAPI media_source_Release(IMFMediaSource *iface) IMFMediaEventQueue_Release(source->event_queue); IMFByteStream_Release(source->byte_stream); wg_parser_destroy(source->wg_parser); + source->cs.DebugInfo->Spare[0] = 0; + DeleteCriticalSection(&source->cs); free(source); }

@@ -1257,27 +1265,39 @@ static HRESULT WINAPI media_source_QueueEvent(IMFMediaSource *iface, MediaEventT static HRESULT WINAPI media_source_GetCharacteristics(IMFMediaSource *iface, DWORD *characteristics) { struct media_source *source = impl_from_IMFMediaSource(iface); + HRESULT hr = S_OK;

TRACE("%p, %p.\n", iface, characteristics);

+ EnterCriticalSection(&source->cs); + if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; + hr = MF_E_SHUTDOWN; + else + *characteristics = MFMEDIASOURCE_CAN_SEEK | MFMEDIASOURCE_CAN_PAUSE;

- *characteristics = MFMEDIASOURCE_CAN_SEEK | MFMEDIASOURCE_CAN_PAUSE; + LeaveCriticalSection(&source->cs);

- return S_OK; + return hr; }

static HRESULT WINAPI media_source_CreatePresentationDescriptor(IMFMediaSource *iface, IMFPresentationDescriptor **descriptor) { struct media_source *source = impl_from_IMFMediaSource(iface); + HRESULT hr;

TRACE("%p, %p.\n", iface, descriptor);

+ EnterCriticalSection(&source->cs); + if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; + hr = MF_E_SHUTDOWN; + else + hr = IMFPresentationDescriptor_Clone(source->pres_desc, descriptor); + + LeaveCriticalSection(&source->cs);

- return IMFPresentationDescriptor_Clone(source->pres_desc, descriptor); + return hr; }

static HRESULT WINAPI media_source_Start(IMFMediaSource *iface, IMFPresentationDescriptor *descriptor, @@ -1289,13 +1309,13 @@ static HRESULT WINAPI media_source_Start(IMFMediaSource *iface, IMFPresentationD

TRACE("%p, %p, %p, %p.\n", iface, descriptor, time_format, position);

- if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; - - if (!(IsEqualIID(time_format, &GUID_NULL))) - return MF_E_UNSUPPORTED_TIME_FORMAT; + EnterCriticalSection(&source->cs);

- if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_START, &command))) + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else if (!(IsEqualIID(time_format, &GUID_NULL))) + hr = MF_E_UNSUPPORTED_TIME_FORMAT; + else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_START, &command))) { command->u.start.descriptor = descriptor; command->u.start.format = *time_format; @@ -1304,6 +1324,8 @@ static HRESULT WINAPI media_source_Start(IMFMediaSource *iface, IMFPresentationD hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, &command->IUnknown_iface); }

+ LeaveCriticalSection(&source->cs); + return hr; }

@@ -1315,12 +1337,15 @@ static HRESULT WINAPI media_source_Stop(IMFMediaSource *iface)

TRACE("%p.\n", iface);

- if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; + EnterCriticalSection(&source->cs);

- if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_STOP, &command))) + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_STOP, &command))) hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, &command->IUnknown_iface);

+ LeaveCriticalSection(&source->cs); + return hr; }

@@ -1332,16 +1357,18 @@ static HRESULT WINAPI media_source_Pause(IMFMediaSource *iface)

TRACE("%p.\n", iface);

- if (source->state == SOURCE_SHUTDOWN) - return MF_E_SHUTDOWN; + EnterCriticalSection(&source->cs);

- if (source->state != SOURCE_RUNNING) - return MF_E_INVALID_STATE_TRANSITION; - - if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_PAUSE, &command))) + if (source->state == SOURCE_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else if (source->state != SOURCE_RUNNING) + hr = MF_E_INVALID_STATE_TRANSITION; + else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_PAUSE, &command))) hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, &command->IUnknown_iface);

+ LeaveCriticalSection(&source->cs); + return S_OK; }

@@ -1351,8 +1378,13 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)

TRACE("%p.\n", iface);

+ EnterCriticalSection(&source->cs); + if (source->state == SOURCE_SHUTDOWN) + { + LeaveCriticalSection(&source->cs); return MF_E_SHUTDOWN; + }

source->state = SOURCE_SHUTDOWN;

@@ -1376,6 +1408,8 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)

MFUnlockWorkQueue(source->async_commands_queue);

+ LeaveCriticalSection(&source->cs); + return S_OK; }

@@ -1435,6 +1469,8 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_ object->byte_stream = bytestream; IMFByteStream_AddRef(bytestream); object->rate = 1.0f; + InitializeCriticalSection(&object->cs); + object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": cs");

if (FAILED(hr = MFCreateEventQueue(&object->event_queue))) goto fail;

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Giovanni Mascellani

9:11 a.m.

New subject: [PATCH v2 5/7] winegstreamer: Synchronize concurrent access to the media stream.

From: Giovanni Mascellani gmascellani@codeweavers.com

--- dlls/winegstreamer/media_source.c | 55 ++++++++++++++++++------------- 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index aadb4d8a42c..ccc13b5e000 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -750,31 +750,46 @@ static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMedi { struct media_stream *stream = impl_from_IMFMediaStream(iface); struct media_source *source = impl_from_IMFMediaSource(stream->media_source); + HRESULT hr = S_OK;

TRACE("%p, %p.\n", iface, out);

+ EnterCriticalSection(&source->cs); + if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; + hr = MF_E_SHUTDOWN; + else + { + IMFMediaSource_AddRef(&source->IMFMediaSource_iface); + *out = &source->IMFMediaSource_iface; + }

- IMFMediaSource_AddRef(&source->IMFMediaSource_iface); - *out = &source->IMFMediaSource_iface; + LeaveCriticalSection(&source->cs);

- return S_OK; + return hr; }

static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IMFStreamDescriptor **descriptor) { struct media_stream *stream = impl_from_IMFMediaStream(iface); + struct media_source *source = impl_from_IMFMediaSource(stream->media_source); + HRESULT hr = S_OK;

TRACE("%p, %p.\n", iface, descriptor);

+ EnterCriticalSection(&source->cs); + if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; + hr = MF_E_SHUTDOWN; + else + { + IMFStreamDescriptor_AddRef(stream->descriptor); + *descriptor = stream->descriptor; + }

- IMFStreamDescriptor_AddRef(stream->descriptor); - *descriptor = stream->descriptor; + LeaveCriticalSection(&source->cs);

- return S_OK; + return hr; }

static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown *token) @@ -786,21 +801,15 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown

TRACE("%p, %p.\n", iface, token);

- if (stream->state == STREAM_SHUTDOWN) - return MF_E_SHUTDOWN; - - if (stream->state == STREAM_INACTIVE) - { - WARN("Stream isn't active\n"); - return MF_E_MEDIA_SOURCE_WRONGSTATE; - } - - if (stream->eos) - { - return MF_E_END_OF_STREAM; - } + EnterCriticalSection(&source->cs);

- if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &command))) + if (stream->state == STREAM_SHUTDOWN) + hr = MF_E_SHUTDOWN; + else if (stream->state == STREAM_INACTIVE) + hr = MF_E_MEDIA_SOURCE_WRONGSTATE; + else if (stream->eos) + hr = MF_E_END_OF_STREAM; + else if (SUCCEEDED(hr = source_create_async_op(SOURCE_ASYNC_REQUEST_SAMPLE, &command))) { command->u.request_sample.stream = stream; if (token) @@ -810,6 +819,8 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown hr = MFPutWorkItem(source->async_commands_queue, &source->async_commands_callback, &command->IUnknown_iface); }

+ LeaveCriticalSection(&source->cs); + return hr; }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Giovanni Mascellani

9:11 a.m.

New subject: [PATCH v2 6/7] winegstreamer: Synchronize access to the media source from callbacks.

From: Giovanni Mascellani gmascellani@codeweavers.com

--- dlls/winegstreamer/media_source.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index ccc13b5e000..edae7a47ac8 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -557,32 +557,36 @@ static HRESULT WINAPI source_async_commands_Invoke(IMFAsyncCallback *iface, IMFA IUnknown *state; HRESULT hr;

- if (source->state == SOURCE_SHUTDOWN) - return S_OK; - if (FAILED(hr = IMFAsyncResult_GetState(result, &state))) return hr;

+ EnterCriticalSection(&source->cs); + command = impl_from_async_command_IUnknown(state); switch (command->op) { case SOURCE_ASYNC_START: - start_pipeline(source, command); + if (source->state != SOURCE_SHUTDOWN) + start_pipeline(source, command); break; case SOURCE_ASYNC_PAUSE: - pause_pipeline(source); + if (source->state != SOURCE_SHUTDOWN) + pause_pipeline(source); break; case SOURCE_ASYNC_STOP: - stop_pipeline(source); + if (source->state != SOURCE_SHUTDOWN) + stop_pipeline(source); break; case SOURCE_ASYNC_REQUEST_SAMPLE: if (source->state == SOURCE_PAUSED) enqueue_token(command->u.request_sample.stream, command->u.request_sample.token); - else + else if (source->state == SOURCE_RUNNING) wait_on_sample(command->u.request_sample.stream, command->u.request_sample.token); break; }

+ LeaveCriticalSection(&source->cs); + IUnknown_Release(state);

return S_OK;

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Rémi Bernon

9:11 a.m.

New subject: [PATCH v2 7/7] winegstreamer: Remove unnecessary media source stream states.

From: Rémi Bernon rbernon@codeweavers.com

--- dlls/winegstreamer/media_source.c | 40 +++++++++++++------------------ 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c index edae7a47ac8..63147d64b32 100644 --- a/dlls/winegstreamer/media_source.c +++ b/dlls/winegstreamer/media_source.c @@ -42,13 +42,8 @@ struct media_stream LONG token_queue_count; LONG token_queue_cap;

- enum - { - STREAM_INACTIVE, - STREAM_SHUTDOWN, - STREAM_RUNNING, - } state; DWORD stream_id; + BOOL active; BOOL eos; };

@@ -350,9 +345,8 @@ static void start_pipeline(struct media_source *source, struct source_async_comm sd = stream_descriptor_from_id(command->u.start.descriptor, stream_id, &selected); IMFStreamDescriptor_Release(sd);

- was_active = stream->state != STREAM_INACTIVE; - - stream->state = selected ? STREAM_RUNNING : STREAM_INACTIVE; + was_active = stream->active; + stream->active = selected;

if (selected) { @@ -404,14 +398,14 @@ static void start_pipeline(struct media_source *source, struct source_async_comm static void pause_pipeline(struct media_source *source) { unsigned int i; + HRESULT hr;

for (i = 0; i < source->stream_count; i++) { struct media_stream *stream = source->streams[i]; - if (stream->state != STREAM_INACTIVE) - { - IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEStreamPaused, &GUID_NULL, S_OK, NULL); - } + if (stream->active && FAILED(hr = IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEStreamPaused, + &GUID_NULL, S_OK, NULL))) + WARN("Failed to queue MEStreamPaused event, hr %#lx\n", hr); }

IMFMediaEventQueue_QueueEventParamVar(source->event_queue, MESourcePaused, &GUID_NULL, S_OK, NULL); @@ -422,12 +416,14 @@ static void pause_pipeline(struct media_source *source) static void stop_pipeline(struct media_source *source) { unsigned int i; + HRESULT hr;

for (i = 0; i < source->stream_count; i++) { struct media_stream *stream = source->streams[i]; - if (stream->state != STREAM_INACTIVE) - IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEStreamStopped, &GUID_NULL, S_OK, NULL); + if (stream->active && FAILED(hr = IMFMediaEventQueue_QueueEventParamVar(stream->event_queue, MEStreamStopped, + &GUID_NULL, S_OK, NULL))) + WARN("Failed to queue MEStreamStopped event, hr %#lx\n", hr); }

IMFMediaEventQueue_QueueEventParamVar(source->event_queue, MESourceStopped, &GUID_NULL, S_OK, NULL); @@ -447,8 +443,7 @@ static void dispatch_end_of_presentation(struct media_source *source) for (i = 0; i < source->stream_count; i++) { struct media_stream *stream = source->streams[i]; - - if (stream->state != STREAM_INACTIVE && !stream->eos) + if (stream->active && !stream->eos) return; }

@@ -760,7 +755,7 @@ static HRESULT WINAPI media_stream_GetMediaSource(IMFMediaStream *iface, IMFMedi

EnterCriticalSection(&source->cs);

- if (stream->state == STREAM_SHUTDOWN) + if (source->state == SOURCE_SHUTDOWN) hr = MF_E_SHUTDOWN; else { @@ -783,7 +778,7 @@ static HRESULT WINAPI media_stream_GetStreamDescriptor(IMFMediaStream* iface, IM

EnterCriticalSection(&source->cs);

- if (stream->state == STREAM_SHUTDOWN) + if (source->state == SOURCE_SHUTDOWN) hr = MF_E_SHUTDOWN; else { @@ -807,9 +802,9 @@ static HRESULT WINAPI media_stream_RequestSample(IMFMediaStream *iface, IUnknown

EnterCriticalSection(&source->cs);

- if (stream->state == STREAM_SHUTDOWN) + if (source->state == SOURCE_SHUTDOWN) hr = MF_E_SHUTDOWN; - else if (stream->state == STREAM_INACTIVE) + else if (!stream->active) hr = MF_E_MEDIA_SOURCE_WRONGSTATE; else if (stream->eos) hr = MF_E_END_OF_STREAM; @@ -867,7 +862,7 @@ static HRESULT media_stream_create(IMFMediaSource *source, DWORD id, object->media_source = source; object->stream_id = id;

- object->state = STREAM_INACTIVE; + object->active = FALSE; object->eos = FALSE; object->wg_stream = wg_parser_get_stream(wg_parser, id);

@@ -1415,7 +1410,6 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) while (source->stream_count--) { struct media_stream *stream = source->streams[source->stream_count]; - stream->state = STREAM_SHUTDOWN; IMFMediaEventQueue_Shutdown(stream->event_queue); IMFMediaStream_Release(&stream->IMFMediaStream_iface); }

-- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Marvin

9:48 a.m.

New subject: [PATCH v2 7/7] winegstreamer: Remove unnecessary media source stream states.

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=132294

Your paranoid android.

=== debian11 (32 bit report) ===

wininet: http: Timeout

Rémi Bernon (＠rbernon)

9:11 a.m.

v2: Dropped the wg_parser changes. They aren't needed if wg_parser_stream_get_buffer can truly never block. I'm not completely sure of that but for now lets assume it is the case.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31625

Zebediah Figura (＠zfigura)

5:32 p.m.

New subject: [PATCH v2 0/7] MR2719: winegstreamer: Cleanup media source streams and add synchronization. - approved

This merge request was approved by Zebediah Figura.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719

Zebediah Figura (＠zfigura)

5:34 p.m.

Looks good to me now, thanks. That said, a lot of this probably desires mfplat knowledge to review, so this probably should have Nikolay's sign-off.

I'm not really sure 2/7 is an improvement, but I suppose it's a matter of taste, so I guess that's up to Nikolay as well.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31670

Nikolay Sivov (＠nsivov)

3 May 3 May

2:46 a.m.

Nikolay Sivov (@nsivov) commented about dlls/winegstreamer/media_source.c:

...

     case SOURCE_ASYNC_PAUSE:

```
       pause_pipeline(source);
```

       if (source->state != SOURCE_SHUTDOWN)

           pause_pipeline(source);
       break;
   case SOURCE_ASYNC_STOP:

```
       stop_pipeline(source);
```

       if (source->state != SOURCE_SHUTDOWN)

           stop_pipeline(source);
       break;
   case SOURCE_ASYNC_REQUEST_SAMPLE:
       if (source->state == SOURCE_PAUSED)
           enqueue_token(command->u.request_sample.stream, command->u.request_sample.token);

```
       else
```

       else if (source->state == SOURCE_RUNNING)
           wait_on_sample(command->u.request_sample.stream, command->u.request_sample.token);

Where does this new state check come from?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31844

Rémi Bernon (＠rbernon)

8 a.m.

On Wed May 3 02:46:54 2023 +0000, Nikolay Sivov wrote:

...

Where does this new state check come from?

What do you mean?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31857

Nikolay Sivov (＠nsivov)

1:27 p.m.

On Wed May 3 08:00:07 2023 +0000, Rémi Bernon wrote:

...

What do you mean?

I mean we need to check for SOURCE_RUNNING explicitly now because of SOURCE_SHUTDOWN? Seems like it would be easier to check for it once and not for every command.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31879

Rémi Bernon (＠rbernon)

1:31 p.m.

On Wed May 3 13:27:26 2023 +0000, Nikolay Sivov wrote:

...

I mean we need to check for SOURCE_RUNNING explicitly now because of SOURCE_SHUTDOWN? Seems like it would be easier to check for it once and not for every command.

Sure, I can do an early exit for now but I think we will want to do things even during shutdown, and it will probably have to be changed later again anyway.

For instance I have some possible plans to replace the read thread with async IMFByteStream reads. I think we might want to keep track of outstanding read / sample requests, decrement the counters here and wait for them in the shutdown procedure.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31880

Nikolay Sivov (＠nsivov)

1:39 p.m.

On Wed May 3 13:31:16 2023 +0000, Rémi Bernon wrote:

...

Sure, I can do an early exit for now but I think we will want to do things even during shutdown, and it will probably have to be changed later again anyway. For instance I have some possible plans to replace the read thread with async IMFByteStream reads. I think we might want to keep track of outstanding read / sample requests, decrement the counters here and wait for them in the shutdown procedure.

Okay, another thing that we should consider I think, is to get rid of separate work queue for every source object. That did create problems before, when application created too many sources failing to respond to errors and exhausted maximum possible number of user queues. There is a MFLockSharedWorkQueue() that could be used here.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31882

Rémi Bernon (＠rbernon)

1:46 p.m.

On Wed May 3 13:39:41 2023 +0000, Nikolay Sivov wrote:

...

Okay, another thing that we should consider I think, is to get rid of separate work queue for every source object. That did create problems before, when application created too many sources failing to respond to errors and exhausted maximum possible number of user queues. There is a MFLockSharedWorkQueue() that could be used here.

Yes, I also want to use the standard queues for the callbacks at some point.

I'm considering using a multi-threaded queue for the sample requests, as it would be simpler to have them wait on a condition variable rather than managing a list, but I'm not completely sure how bad it would be to occupy a thread for each pending request.

As an alternative I have seen that there's some event-based work items, which could probably be used without unnecessary resource occupation.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31883

Zebediah Figura (＠zfigura)

3:42 p.m.

...

For instance I have some possible plans to replace the read thread with async IMFByteStream reads. I think we might want to keep track of outstanding read / sample requests, decrement the counters here and wait for them in the shutdown procedure.

What's the motivation for this?

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31888

Rémi Bernon (＠rbernon)

4:10 p.m.

On Wed May 3 15:42:29 2023 +0000, Zebediah Figura wrote:

...

...
For instance I have some possible plans to replace the read thread

with async IMFByteStream reads. I think we might want to keep track of outstanding read / sample requests, decrement the counters here and wait for them in the shutdown procedure. What's the motivation for this?

No real motivation yet, just something that would feel natural in the MF ecosystem. Potentially it would reduce the number of threads by having the work queues pool them.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31893

Zebediah Figura (＠zfigura)

4:56 p.m.

On Wed May 3 16:09:59 2023 +0000, Rémi Bernon wrote:

...

No real motivation yet, just something that would feel natural in the MF ecosystem. Potentially it would reduce the number of threads by having the work queues pool them.

If it makes the mfplat code simpler, then I don't see anything wrong with it. Note though that file I/O is always synchronous in Wine, so async I/O won't actually improve performance at all, and will actually slightly degrade it instead.

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2719#note_31898

650

Age (days ago)

652

Last active (days ago)

wine-gitlab@winehq.org

20 comments

6 participants

tags (0)

participants (6)

Giovanni Mascellani
Marvin
Nikolay Sivov (＠nsivov)
Rémi Bernon
Rémi Bernon (＠rbernon)
Zebediah Figura (＠zfigura)