Re: [PATCH 5/5] winegstreamer: Implement async reader Open / Close asynchronously.

[Zebediah Figura]

On 9/14/22 05:01, Rémi Bernon wrote:
> @@ -236,6 +243,26 @@ static DWORD WINAPI async_reader_callback_thread(void *arg)
>               hr = list_empty(&reader->async_ops) ? S_OK : E_ABORT;
>               switch (op->type)
>               {
> -
>            case ASYNC_OP_OPEN_FILE:
> -
>                if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_file(&reader->reader, op->u.open_file.filename)))
> -
>                    reader->running = false;
> -
>                free(op->u.open_file.filename);
> -
>                goto case_ASYNC_OP_OPEN;
> -
> -
>            case ASYNC_OP_OPEN_STREAM:
> -
>                if (SUCCEEDED(hr) && FAILED(hr = wm_reader_open_stream(&reader->reader, op->u.open_stream.stream)))
> -
>                    reader->running = false;
> -
>                IStream_Release(op->u.open_stream.stream);
> -
>                goto case_ASYNC_OP_OPEN;
> -
> -
>            case_ASYNC_OP_OPEN:
> -
>                reader->opened = SUCCEEDED(hr);
> -
>                LeaveCriticalSection(&reader->callback_cs);
> -
>                IWMReaderCallback_OnStatus(reader->callback, WMT_OPENED, hr,
> -
>                        WMT_TYPE_DWORD, (BYTE *)&zero, reader->context);
> -
>                EnterCriticalSection(&reader->callback_cs);
> -
>                break;
> -

Please don't do this, it's unidiomatic and (especially with the current 
naming) misleading. In this case I think a helper function would be fine.
>               case ASYNC_OP_START:
>               {
>                   reader->context = op->u.start.context;
>
> @@ -282,6 +309,10 @@ static DWORD WINAPI async_reader_callback_thread(void *arg)
>   LeaveCriticalSection(&reader->callback_cs);
>
> - if (reader->opened)
> -
>    wm_reader_close(&reader->reader);
> - reader->opened = false;
> -
> TRACE("Reader is stopping; exiting.\n");
> return 0;
> }
>
> @@ -386,18 +417,26 @@ static HRESULT WINAPI WMReader_Open(IWMReader *iface, const WCHAR *url,
>           IWMReaderCallback *callback, void *context)
>   {
>       struct async_reader *reader = impl_from_IWMReader(iface);
> -
>
> union async_op_data data;
>  HRESULT hr;
>
> TRACE("reader %p, url %s, callback %p, context %p.\n",
>          reader, debugstr_w(url), callback, context);
> -
>
> if (!(data.open_file.filename = wcsdup(url)))
> -
>    return E_OUTOFMEMORY;
> -

It's admittedly not exactly obvious to me why we need to execute 
wm_reader_open_file() / wm_reader_open_stream() on the callback thread. 
It wouldn't surprise me if that's how native behaves (not that I'm sure 
there's any way to tell?) and it doesn't seem *wrong* either, but it'd 
be nice to have clearer motivation, especially since it seems to add a 
bit of extra complexity.
>   EnterCriticalSection(&reader->cs);
>
> - if (SUCCEEDED(hr = wm_reader_open_file(&reader->reader, url))
> -
>        && FAILED(hr = async_reader_open(reader, callback, context)))
> -
>    wm_reader_close(&reader->reader);
> - if (reader->opened)
> -
>    hr = E_UNEXPECTED;

We set reader->opened on the callback thread, but check it from the 
application thread. This actually *is* correct, I think (hooray, 
Windows), but it looks wrong, so it could do with a comment.

That or we just implement it the correct-looking way anyway, i.e. always 
set it from the application thread. In that case I don't think we even 
need the "opened" variable, but instead can use 
"reader->callback_thread" like in IWMReader::Close(). Having symmetry 
there probably wouldn't hurt either way.