[PATCH v3 0/2] MR3510: Cast duration to double to avoid precision loss.

[Zhiyi Zhang (＠zhiyi)]

--
  v3: mfmediaengine: Use a mftime_to_second() helper to convert time.
      mfmediaengine/tests: Test IMFMediaEngine::GetDuration().

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

[PATCH v3 1/2] mfmediaengine/tests: Test IMFMediaEngine::GetDuration().

[Zhiyi Zhang]

From: Zhiyi Zhang

---
 dlls/mfmediaengine/tests/mfmediaengine.c | 49 ++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
index aa9de414fcb..7124eda3c8c 100644
--- a/dlls/mfmediaengine/tests/mfmediaengine.c
+++ b/dlls/mfmediaengine/tests/mfmediaengine.c
@@ -17,6 +17,7 @@
  */

#include <stdarg.h>
+#include <math.h>

#define COBJMACROS

@@ -66,6 +67,11 @@ static void check_interface_(unsigned int line, void *iface_ptr, REFIID iid, BOO
         IUnknown_Release(unk);
 }

+static BOOL compare_double(double a, double b, double allowed_error)
+{
+    return fabs(a - b) <= allowed_error;
+}
+
 static DWORD compare_rgb32(const BYTE *data, DWORD *length, const RECT *rect, const BYTE *expect)
 {
     DWORD x, y, size, diff = 0, width = (rect->right + 0xf) & ~0xf, height = (rect->bottom + 0xf) & ~0xf;
@@ -1925,6 +1931,48 @@ done:
     IMFMediaEngineNotify_Release(&notify->IMFMediaEngineNotify_iface);
 }

+static void test_GetDuration(void)
+{
+    static const double allowed_error = 0.000001;
+    struct test_transfer_notify *notify;
+    IMFMediaEngineEx *media_engine;
+    IMFByteStream *stream;
+    ID3D11Device *device;
+    double duration;
+    HRESULT hr;
+    DWORD res;
+    BSTR url;
+
+    if (!(device = create_d3d11_device()))
+    {
+        skip("Failed to create a D3D11 device, skipping tests.\n");
+        return;
+    }
+
+    notify = create_transfer_notify();
+    media_engine = create_media_engine_ex(&notify->IMFMediaEngineNotify_iface, NULL, DXGI_FORMAT_B8G8R8X8_UNORM);
+    notify->media_engine = media_engine;
+    ok(!!media_engine, "create_media_engine_ex failed.\n");
+
+    stream = load_resource(L"i420-64x64.avi", L"video/avi");
+    url = SysAllocString(L"i420-64x64.avi");
+    hr = IMFMediaEngineEx_SetSourceFromByteStream(media_engine, stream, url);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    res = WaitForSingleObject(notify->ready_event, 5000);
+    ok(!res, "Unexpected res %#lx.\n", res);
+
+    duration = IMFMediaEngineEx_GetDuration(media_engine);
+    todo_wine
+    ok(compare_double(duration, 0.133467, allowed_error), "Got unexpected duration %lf.\n", duration);
+
+    SysFreeString(url);
+    IMFByteStream_Release(stream);
+    IMFMediaEngineEx_Shutdown(media_engine);
+    IMFMediaEngineEx_Release(media_engine);
+    IMFMediaEngineNotify_Release(&notify->IMFMediaEngineNotify_iface);
+    ID3D11Device_Release(device);
+}
+
 START_TEST(mfmediaengine)
 {
     HRESULT hr;
@@ -1957,6 +2005,7 @@ START_TEST(mfmediaengine)
     test_audio_configuration();
     test_TransferVideoFrame();
     test_effect();
+    test_GetDuration();

IMFMediaEngineClassFactory_Release(factory);

[PATCH v3 2/2] mfmediaengine: Use a mftime_to_second() helper to convert time.

[Zhiyi Zhang]

From: Zhiyi Zhang

---
 dlls/mfmediaengine/main.c                | 13 ++++++++-----
 dlls/mfmediaengine/tests/mfmediaengine.c |  1 -
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c
index e664db81d1d..bc7a131f594 100644
--- a/dlls/mfmediaengine/main.c
+++ b/dlls/mfmediaengine/main.c
@@ -63,6 +63,12 @@ static BOOL mf_array_reserve(void **elements, size_t *capacity, size_t count, si
     return TRUE;
 }

+/* Convert 100ns to seconds */
+static double mftime_to_second(MFTIME time)
+{
+    return (double)time / 10000000.0;
+}
+
 enum media_engine_mode
 {
     MEDIA_ENGINE_INVALID,
@@ -1248,10 +1254,7 @@ static HRESULT media_engine_create_topology(struct media_engine *engine, IMFMedi

/* Assume live source if duration was not provided. */
     if (SUCCEEDED(IMFPresentationDescriptor_GetUINT64(pd, &MF_PD_DURATION, &duration)))
-    {
-        /* Convert 100ns to seconds. */
-        engine->duration = duration / 10000000;
-    }
+        engine->duration = mftime_to_second(duration);
     else
         engine->duration = INFINITY;

@@ -1749,7 +1752,7 @@ static double WINAPI media_engine_GetCurrentTime(IMFMediaEngineEx *iface)
     }
     else if (SUCCEEDED(IMFPresentationClock_GetTime(engine->clock, &clocktime)))
     {
-        ret = (double)clocktime / 10000000.0;
+        ret = mftime_to_second(clocktime);
     }
     LeaveCriticalSection(&engine->cs);

diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
index 7124eda3c8c..31356977f2a 100644
--- a/dlls/mfmediaengine/tests/mfmediaengine.c
+++ b/dlls/mfmediaengine/tests/mfmediaengine.c
@@ -1962,7 +1962,6 @@ static void test_GetDuration(void)
     ok(!res, "Unexpected res %#lx.\n", res);

duration = IMFMediaEngineEx_GetDuration(media_engine);
-    todo_wine
     ok(compare_double(duration, 0.133467, allowed_error), "Got unexpected duration %lf.\n", duration);

SysFreeString(url);

[PATCH v3 2/2] mfmediaengine: Use a mftime_to_second() helper to convert time.

[Marvin]

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

Your paranoid android.

=== w10pro64_zh_CN (64 bit report) ===

mfmediaengine:
mfmediaengine.c:1912: Test failed: Unexpected ref 4.
mfmediaengine.c:1917: Test failed: Unexpected ref 4.
mfmediaengine.c:1923: Test failed: Unexpected ref 1.

[Jeffrey Smith (＠whydoubt)]

On Tue Aug  8 02:50:05 2023 +0000, Zhiyi Zhang wrote:
> changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/3510/diffs?diff_id=61485&start_sha=2ce2222b25f632aa27dfcabc08d86ed581846d12#238bb11c24dbacb840d4252cb92aa8ea2d26e240_1253_1257)

Totally agree with the helper.
Agree to disagree on readability. Actually I generally favor chunking (done with _ in some programming languages) over exponential notation, but C obviously doesn't have that.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/mfmediaengine/main.c:
>  return TRUE;
>
> }
>
> +/* Convert 100ns to seconds */
> +static double mftime_to_second(MFTIME time)
> +{
> - return (double)time / 10000000.0;

First, I like this helper.

Because the second term is now a `double`, `time` will be automatically promoted.
So we don't need the explicit cast, but do we want to keep it anyway?

[Mingye Wang (＠Artoria2e5)]

On Tue Aug  8 03:04:43 2023 +0000, Jeffrey Smith wrote:
> Totally agree with the helper.
> Agree to disagree on readability. Actually I generally favor chunking
> (done with _ in some programming languages) over exponential notation,
> but C obviously doesn't have that.

You can actually use `10'000'000` in C... C23, but wine cares about supporting platforms that aren't on C23 yet.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
> +{
> - static const double allowed_error = 0.000001;
> - struct test_transfer_notify *notify;
> - IMFMediaEngineEx *media_engine;
> - IMFByteStream *stream;
> - ID3D11Device *device;
> - double duration;
> - HRESULT hr;
> - DWORD res;
> - BSTR url;
> -
> - if (!(device = create_d3d11_device()))
> - {
> -
>    skip("Failed to create a D3D11 device, skipping tests.\n");
> -
>    return;
> - }

Thanks. A few comments, you don't need a d3d device now.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/main.c:
>  return TRUE;
>
> }
>
> +/* Convert 100ns to seconds */
> +static double mftime_to_second(MFTIME time)

Let's call it mftime_to_seconds().