[PATCH 0/1] MR4943: mfreadwrite: Replace presentation clock with clock state sink for writer.

[Ziqing Hui (＠zhui)]

There is no need to store a presentation clock object.

[PATCH 1/1] mfreadwrite: Replace presentation clock with clock state sink for writer.

[Ziqing Hui]

From: Ziqing Hui

There is no need to store a presentation clock object.
---
 dlls/mfreadwrite/writer.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/dlls/mfreadwrite/writer.c b/dlls/mfreadwrite/writer.c
index c708664d2fc..b037c7901de 100644
--- a/dlls/mfreadwrite/writer.c
+++ b/dlls/mfreadwrite/writer.c
@@ -77,7 +77,6 @@ struct sink_writer
         DWORD next_id;
     } streams;

-    IMFPresentationClock *clock;
     IMFMediaSink *sink;
     enum writer_state state;
     HRESULT status;
@@ -221,8 +220,6 @@ static ULONG WINAPI sink_writer_Release(IMFSinkWriter *iface)

if (!refcount)
     {
-        if (writer->clock)
-            IMFPresentationClock_Release(writer->clock);
         if (writer->sink)
             IMFMediaSink_Release(writer->sink);
         if (writer->callback)
@@ -312,28 +309,10 @@ static HRESULT WINAPI sink_writer_SetInputMediaType(IMFSinkWriter *iface, DWORD
     return E_NOTIMPL;
 }

-static HRESULT sink_writer_set_presentation_clock(struct sink_writer *writer)
-{
-    IMFPresentationTimeSource *time_source = NULL;
-    HRESULT hr;
-
-    if (FAILED(hr = MFCreatePresentationClock(&writer->clock))) return hr;
-
-    if (FAILED(IMFMediaSink_QueryInterface(writer->sink, &IID_IMFPresentationTimeSource, (void **)&time_source)))
-        hr = MFCreateSystemTimeSource(&time_source);
-
-    if (SUCCEEDED(hr = IMFPresentationClock_SetTimeSource(writer->clock, time_source)))
-        hr = IMFMediaSink_SetPresentationClock(writer->sink, writer->clock);
-
-    if (time_source)
-        IMFPresentationTimeSource_Release(time_source);
-
-    return hr;
-}
-
 static HRESULT WINAPI sink_writer_BeginWriting(IMFSinkWriter *iface)
 {
     struct sink_writer *writer = impl_from_IMFSinkWriter(iface);
+    IMFClockStateSink *state_sink = NULL;
     HRESULT hr = S_OK;
     unsigned int i;

@@ -341,11 +320,9 @@ static HRESULT WINAPI sink_writer_BeginWriting(IMFSinkWriter *iface)

EnterCriticalSection(&writer->cs);

-    if (!writer->streams.count)
-        hr = MF_E_INVALIDREQUEST;
-    else if (writer->state != SINK_WRITER_STATE_INITIAL)
+    if (!writer->streams.count || writer->state != SINK_WRITER_STATE_INITIAL)
         hr = MF_E_INVALIDREQUEST;
-    else if (SUCCEEDED(hr = sink_writer_set_presentation_clock(writer)))
+    else if (SUCCEEDED(hr = IMFMediaSink_QueryInterface(writer->sink, &IID_IMFClockStateSink, (void **)&state_sink)))
     {
         for (i = 0; i < writer->streams.count; ++i)
         {
@@ -362,11 +339,14 @@ static HRESULT WINAPI sink_writer_BeginWriting(IMFSinkWriter *iface)
         }

if (SUCCEEDED(hr))
-            hr = IMFPresentationClock_Start(writer->clock, 0);
+            hr = IMFClockStateSink_OnClockStart(state_sink, MFGetSystemTime(), 0);

writer->state = SINK_WRITER_STATE_WRITING;
     }

+    if (state_sink)
+        IMFClockStateSink_Release(state_sink);
+
     LeaveCriticalSection(&writer->cs);

return hr;

[Nikolay Sivov (＠nsivov)]

How did you verify this?

[Ziqing Hui (＠zhui)]

On Sat Jan 27 02:36:33 2024 +0000, Nikolay Sivov wrote:
> How did you verify this?

In our writer, the presentation clock is only needed in BeginWriting(). The purpose of it is to start the media sink, making media sink get into started state. And I don't find any other usages of the presentation clock (Did I miss other usages?).

To start media sink, calling OnClockStart() is enough, we don't need presentation clock.

[Ziqing Hui (＠zhui)]

On Sat Jan 27 02:36:33 2024 +0000, Ziqing Hui wrote:
> In our writer, the presentation clock is only needed in BeginWriting().
> The purpose of it is to start the media sink, making media sink get into
> started state. And I don't find any other usages of the presentation
> clock (Did I miss other usages?). 
> To start media sink, calling OnClockStart() is enough, we don't need
> presentation clock.

I think we don't need presentation clock to do operations on media sink. We can do things directly from the interfaces quried from media sink.

[Ziqing Hui (＠zhui)]

On Sat Jan 27 02:38:23 2024 +0000, Ziqing Hui wrote:
> I think we don't need presentation clock to do operations on media sink.
> We can do things directly from the interfaces quried from media sink.

MSDN says media sink should always have IMFClockStateSink interface exposed:

https://learn.microsoft.com/en-us/windows/win32/medfound/media-sinks#media-s...

[Ziqing Hui (＠zhui)]

On Sat Jan 27 02:43:55 2024 +0000, Ziqing Hui wrote:
> MSDN says media sink should always have IMFClockStateSink interface exposed:
> https://learn.microsoft.com/en-us/windows/win32/medfound/media-sinks#media-s...

Calling IMFPresentationClock_Start() on presentation clock finally get it invoke IMFClockStateSink_OnClockStart() which is what media sink truely need.

[Nikolay Sivov (＠nsivov)]

On Sat Jan 27 02:47:39 2024 +0000, Ziqing Hui wrote:
> Calling IMFPresentationClock_Start() on presentation clock finally get
> it invoke IMFClockStateSink_OnClockStart() which is what media sink
> truely need.

The only thing that matters is whether the clock is set on Windows. That's why I asked if you verified this change.

[Ziqing Hui (＠zhui)]

On Mon Jan 29 09:35:05 2024 +0000, Nikolay Sivov wrote:
> The only thing that matters is whether the clock is set on Windows.
> That's why I asked if you verified this change.

OK, I made some tests today. Looks like GetPresentationClock() returns a valid clock. So the clock is set on Windows. I'll close this MR.

[PATCH 0/1] MR4943: mfreadwrite: Replace presentation clock with clock state sink for writer. - closed

[Ziqing Hui (＠zhui)]

This merge request was closed by Ziqing Hui.

[Nikolay Sivov (＠nsivov)]

On Tue Jan 30 08:40:07 2024 +0000, Ziqing Hui wrote:
> OK, I made some tests today. Looks like GetPresentationClock() returns a
> valid clock. So the clock is set on Windows. I'll close this MR.

You can also use MFCreateSinkWriterFromMediaSink() with a custom sink stub and see what happens. It's a legitimate use case, because sinks are supposed to be external plugin-like objects.

[Ziqing Hui (＠zhui)]

On Tue Jan 30 08:50:22 2024 +0000, Nikolay Sivov wrote:
> You can also use MFCreateSinkWriterFromMediaSink() with a custom sink
> stub and see what happens. It's a legitimate use case, because sinks are
> supposed to be external plugin-like objects.

I created a writer from mp4 media sink. Before calling BeginWriting() on writer, GetPresentationClock() on that sink returned MF_E_NO_CLOCK. After BeginWriting(), it returns a valid clock.

The test code is attacted: [clock_test.c](/uploads/27be06ce75c1c05e51fe04a6bde08b13/clock_test.c).

[Ziqing Hui (＠zhui)]

On Wed Jan 31 02:23:16 2024 +0000, Ziqing Hui wrote:
> I created a writer from mp4 media sink. Before calling BeginWriting() on
> writer, GetPresentationClock() on that sink returned MF_E_NO_CLOCK.
> After BeginWriting(), it returns a valid clock.
> The test code is attacted: [clock_test.c](/uploads/27be06ce75c1c05e51fe04a6bde08b13/clock_test.c).

Oh, I got a question not relating to this MR:  Where did the sink class factory guids in mfinternal.idl come from? Did we generate it by ourself?