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

[Zhiyi Zhang (＠zhiyi)]

--
  v2: mfmediaengine: Cast duration to double to avoid precision loss.

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

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

[Zhiyi Zhang]

From: Zhiyi Zhang

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

diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
index aa9de414fcb..a0d355c1ec1 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,57 @@ 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;
+    IMFDXGIDeviceManager *manager;
+    IMFByteStream *stream;
+    ID3D11Device *device;
+    double duration;
+    HRESULT hr;
+    UINT token;
+    DWORD res;
+    BSTR url;
+
+    if (!(device = create_d3d11_device()))
+    {
+        skip("Failed to create a D3D11 device, skipping tests.\n");
+        return;
+    }
+
+    hr = pMFCreateDXGIDeviceManager(&token, &manager);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    hr = IMFDXGIDeviceManager_ResetDevice(manager, (IUnknown *)device, token);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    notify = create_transfer_notify();
+    media_engine = create_media_engine_ex(&notify->IMFMediaEngineNotify_iface, manager,
+            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);
+    IMFDXGIDeviceManager_Release(manager);
+    ID3D11Device_Release(device);
+}
+
 START_TEST(mfmediaengine)
 {
     HRESULT hr;
@@ -1957,6 +2014,7 @@ START_TEST(mfmediaengine)
     test_audio_configuration();
     test_TransferVideoFrame();
     test_effect();
+    test_GetDuration();

IMFMediaEngineClassFactory_Release(factory);

[PATCH v2 2/2] mfmediaengine: Cast duration to double to avoid precision loss.

[Zhiyi Zhang]

From: Zhiyi Zhang

---
 dlls/mfmediaengine/main.c                | 2 +-
 dlls/mfmediaengine/tests/mfmediaengine.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/dlls/mfmediaengine/main.c b/dlls/mfmediaengine/main.c
index e664db81d1d..ebef5392128 100644
--- a/dlls/mfmediaengine/main.c
+++ b/dlls/mfmediaengine/main.c
@@ -1250,7 +1250,7 @@ static HRESULT media_engine_create_topology(struct media_engine *engine, IMFMedi
     if (SUCCEEDED(IMFPresentationDescriptor_GetUINT64(pd, &MF_PD_DURATION, &duration)))
     {
         /* Convert 100ns to seconds. */
-        engine->duration = duration / 10000000;
+        engine->duration = (double)duration / 1e7;
     }
     else
         engine->duration = INFINITY;
diff --git a/dlls/mfmediaengine/tests/mfmediaengine.c b/dlls/mfmediaengine/tests/mfmediaengine.c
index a0d355c1ec1..a403d1127a8 100644
--- a/dlls/mfmediaengine/tests/mfmediaengine.c
+++ b/dlls/mfmediaengine/tests/mfmediaengine.c
@@ -1970,7 +1970,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);

[Zhiyi Zhang (＠zhiyi)]

On Fri Aug  4 12:42:54 2023 +0000, Bernhard Kölbl wrote:
> This test duplicates a bunch of code, so not sure, but maybe we could
> integrate this in the TransferVideoFrames test?

We could. But I like the tests clearly separated.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/tests/mfmediaengine.c:
> - if (!(device = create_d3d11_device()))
> - {
> -
>    skip("Failed to create a D3D11 device, skipping tests.\n");
> -
>    return;
> - }
> -
> - hr = pMFCreateDXGIDeviceManager(&token, &manager);
> - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> - hr = IMFDXGIDeviceManager_ResetDevice(manager, (IUnknown *)device, token);
> - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> -
> - notify = create_transfer_notify();
> - media_engine = create_media_engine_ex(&notify->IMFMediaEngineNotify_iface, manager,
> -
>        DXGI_FORMAT_B8G8R8X8_UNORM);
> - notify->media_engine = media_engine;
> - ok(!!media_engine, "create_media_engine_ex failed.\n");

For GetDuration() test you don't need a d3d11 device/device manager. It would be better to have a single notification callback implementation, but we can deal with that later.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/mfmediaengine/main.c:
>  if (SUCCEEDED(IMFPresentationDescriptor_GetUINT64(pd, &MF_PD_DURATION, &duration)))
>  {
>      /* Convert 100ns to seconds. */
> -
>    engine->duration = duration / 10000000;
> -
>    engine->duration = (double)duration / 1e7;

We already have the same expression in media_engine_GetCurrentTime(), I think we can reuse that with a helper. I don't find 1e7 easier to read in this case.