Re: [PATCH 5/6] amstream: Implement audio stream sample Update.

[Zebediah Figura]

Hello Anton, nice work on the patch! It mostly looks good to me; I have
some comments and questions inlined.

On 4/18/20 9:34 AM, Anton Baskanov wrote:
> +static void process_update(IAudioStreamSampleImpl *sample, struct queued_event *event)
> +{
> - DWORD advance;
> -
> - if (event->type == QET_END_OF_STREAM)
> - {
> -
>    sample->update_hr = sample->position ? S_OK : MS_S_ENDOFSTREAM;
> -
>    return;
> - }
> -
> - advance = min(event->length - event->position, sample->length - sample->position);
> - CopyMemory(&sample->pointer[sample->position], &event->pointer[event->position], advance);

memcpy()?
> -
> - event->position += advance;
> - sample->position += advance;
> -
> - sample->update_hr = (sample->position == sample->length) ? S_OK : MS_S_PENDING;
>
> +}
> -
>
> +static void process_updates(struct audio_stream *stream)
> +{
> - while (!list_empty(&stream->update_queue) && !list_empty(&stream->event_queue))
> - {
> -
>    IAudioStreamSampleImpl *sample = LIST_ENTRY(list_head(&stream->update_queue), IAudioStreamSampleImpl, entry);
> -
>    struct queued_event *event = LIST_ENTRY(list_head(&stream->event_queue), struct queued_event, entry);
> -
> -
>    process_update(sample, event);
> -
> -
>    if (MS_S_PENDING != sample->update_hr)
> -
>        remove_queued_update(sample);
> -
>    if ((event->type != QET_END_OF_STREAM) && (event->position == event->length))
> -
>        remove_queued_event(event);

It kind of feels weird to me to queue EOS events the same way as
samples, given they apply to all streams at once, can't be interleaved,
can't be sent multiple times, and don't get removed from the queue until
the stream is stopped (or flushed, presumably?).

Maybe it would be clearer not to queue EOS events at all, but instead to
loop through all queued samples in audio_sink_EndOfStream(), completing
them with MS_E_ENDOFSTREAM, and similarly to check for stream->eos in
process_updates(). I imagine you could get rid of the queued_event_type
enumeration then, unless there's another kind of event that I'm not
anticipating?
> - }
>
> +}
> -
>
> static inline struct audio_stream *impl_from_IAMMediaStream(IAMMediaStream *iface)
>  {
>      return CONTAINING_RECORD(iface, struct audio_stream, IAMMediaStream_iface);
> @@ -449,6 +503,85 @@ static inline struct audio_stream *impl_from_IAudioMediaStream(IAudioMediaStream
>      return CONTAINING_RECORD(iface, struct audio_stream, IAudioMediaStream_iface);
>  }
>
> +static HRESULT WINAPI IAudioStreamSampleImpl_Update(IAudioStreamSample *iface, DWORD flags, HANDLE event,
> -
>    PAPCFUNC func_APC, DWORD APC_data)
>
> +{
> - IAudioStreamSampleImpl *sample = impl_from_IAudioStreamSample(iface);
> - struct audio_stream *stream;
> - DWORD length;
> - BYTE *pointer;
> - HRESULT hr;
> -
> - TRACE("(%p)->(%x,%p,%p,%u)\n", iface, flags, event, func_APC, APC_data);
> -
> - stream = impl_from_IAudioMediaStream(sample->parent);

Perhaps easier would be to take patch 2/6 a step further and store
sample->parent as a pointer to struct audio_stream. Then you wouldn't
need to move this function out of place.
> -
> - hr = IAudioData_GetInfo(sample->audio_data, &length, &pointer, NULL);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - if (event && func_APC)
> -
>    return E_INVALIDARG;
> -
> - if (func_APC)
> - {
> -
>    FIXME("APC support is not implemented!\n");
> -
>    return E_NOTIMPL;
> - }
> -
> - if (event)
> - {
> -
>    FIXME("Event parameter support is not implemented!\n");
> -
>    return E_NOTIMPL;
> - }
> -
> - if (flags & ~SSUPDATE_ASYNC)
> - {
> -
>    FIXME("Unsupported flags: %x\n", flags);
> -
>    return E_NOTIMPL;
> - }
> -
> - EnterCriticalSection(&stream->cs);
> -
> - if (stream->state == State_Stopped)
> - {
> -
>    LeaveCriticalSection(&stream->cs);
> -
>    return MS_E_NOTRUNNING;
> - }

Do you have tests for how Update() behaves while paused? If it's as
simple as "identical to stopped or running", it'd be nice to add a quick
test below.
> - if (!stream->peer)
> - {
> -
>    LeaveCriticalSection(&stream->cs);
> -
>    return MS_S_ENDOFSTREAM;
> - }
> - if (MS_S_PENDING == sample->update_hr)
> - {
> -
>    LeaveCriticalSection(&stream->cs);
> -
>    return MS_E_BUSY;
> - }
> -
> - sample->length = length;
> - sample->pointer = pointer;
> - sample->position = 0;
> - sample->update_hr = MS_S_PENDING;
> - ResetEvent(sample->update_event);
> - list_add_tail(&stream->update_queue, &sample->entry);
> -
> - process_updates(stream);
> -
> - hr = sample->update_hr;
> - if (hr != MS_S_PENDING || (flags & SSUPDATE_ASYNC))
> - {
> -
>    LeaveCriticalSection(&stream->cs);
> -
>    return hr;
> - }
> -
> - LeaveCriticalSection(&stream->cs);
> -
> - WaitForSingleObject(sample->update_event, INFINITE);
> -
> - return sample->update_hr;
>
> +}
> -