[PATCH v1 3/3] mfreadwrite: Implement MF_SOURCE_READER_ASYNC_CALLBACK.
Nikolay Sivov
nsivov at codeweavers.com
Wed Mar 18 08:37:52 CDT 2020
On 3/17/20 7:48 PM, Derek Lesho wrote:
> @@ -587,6 +594,8 @@ static ULONG WINAPI src_reader_Release(IMFSourceReader *iface)
> list_remove(&ptr->entry);
> heap_free(ptr);
> }
> +
> + MFUnlockWorkQueue(stream->read_samples_queue);
> }
> heap_free(reader->streams);
> DeleteCriticalSection(&reader->cs);
Is additional queue necessary for synchronous case?
> + if (FAILED(hr = media_stream_get_id(state, &id)))
> + {
> + WARN("Bad stream %p, hr %#x.\n", state, hr);
> + }
> +
> + for (unsigned int i = 0; i < reader->stream_count; ++i)
> + {
> + if (id == reader->streams[i].id)
> + {
> + struct media_stream *stream = &reader->streams[i];
> + IMFSample *sample = NULL;
> + DWORD stream_flags;
> + LONGLONG timestamp = 0;
> +
> + hr = next_sample(stream, &sample, &stream_flags, FALSE);
> + if (sample)
> + {
> + IMFSample_GetSampleTime(sample, ×tamp);
> + }
> +
> + TRACE("Invoking read sample callback %p with (hr = %#x, stream_idx = %u, flags = %#x, timestamp %lu, sample %p)\n", reader->async_callback, hr, i, stream_flags, timestamp, sample);
> + hr = IMFSourceReaderCallback_OnReadSample(reader->async_callback, hr, i, stream_flags, timestamp, sample);
> + IMFSample_Release(sample);
> + return hr;
> + }
> + }
> +
> + return S_OK;
We already talked about this part - blocking in next_sample(), even on
dedicated thread, is unnecessary.
What will happen is MENewSample from stream queue -> wake thread in
dedicated per-stream queue -> process -> call OnReadSample.
Without blocking you'll have MENewSample -> process on same event
handling thread -> call OnReadSample.
I don't see a need for additional complexity.
> + switch (index)
> + {
> + case MF_SOURCE_READER_FIRST_VIDEO_STREAM:
> + stream_index = reader->first_video_stream_index;
> + break;
> + case MF_SOURCE_READER_FIRST_AUDIO_STREAM:
> + stream_index = reader->first_audio_stream_index;
> + break;
> + case MF_SOURCE_READER_ANY_STREAM:
> + FIXME("Non-specific requests are not supported.\n");
> + return E_NOTIMPL;
> + default:
> + stream_index = index;
> + }
> +
> + /* Can't read from deselected streams. */
> + if (FAILED(hr = source_reader_get_stream_selection(reader, stream_index, &selected)) && !selected)
> + return hr;
If that's how it works, this part applies to both sync and async cases.
> + EnterCriticalSection(&stream->cs);
> + while(!stream->stream)
> + {
> + SleepConditionVariableCS(&stream->sample_event, &stream->cs, INFINITE);
> + }
> + LeaveCriticalSection(&stream->cs);
> +
> + TRACE("Dispatching read sample callback for stream %p\n", stream->stream);
> + if (FAILED(hr = MFPutWorkItem(stream->read_samples_queue, &reader->read_samples_callback, (IUnknown*)stream->stream)))
> + {
> + WARN("Failed to submit item hr = %#x\n", hr);
> + return E_FAIL;
> + }
This blocks for indefinite amount of time, for ReadSample method that's
supposed to be non-blocking. It's correct of course to start the source
here,
which also could be potentially shared with sync case, but waiting for
new samples, using 'sample_event' that's not meant for it, is wrong.
It should start source, and keep a note of number of times each stream
was requested, later on MENewStream/MEUpdatedStream you can issue that
many requests at once.
More information about the wine-devel
mailing list