[PATCH v2 0/1] MR5157: evr/dshow: List NV12 as supported, improve warnings

[David Gow (＠sulix)]

Add support for NV12 to ``evr_render()``, as otherwise no video is rendered
at all in games like Age of Empires II DE (see: https://github.com/ValveSoftware/Proton/issues/3189#issuecomment-1962984985)

Interestingly, this doesn't result in NV12 data being passed to ``evr_copy_sample_buffer()`` as I suspected, so the RGB32 copying code seems to work fine.

Nevertheless, add a warning if we get an unknown format in ``evr_copy_sample_buffer()``, as that could potentially lead to nasty memory
issues (e.g., if we get the width/lines/stride wrong).


I confess, I don't really understand what's going on here: the format of the video is definitely ``MFVideoFormat_NV12`` in ``evr_render()``, but I never see any attempt to use NV12 in ``evr_copy_sample_buffer()`` — it's always ``MFVideoFormat_RGB32``. For that reason, I've also not tried to write any tests here — I don't know the MF API well enough to plumb this all the way through. On the bright side, though, the fact that everything mysteriously ends up as RGB32 means I haven't had to write an NV12 codepath for ``evr_copy_sample_buffer()`` — I'm not sure exactly how to handle the multi-plane formats there.

--
  v2: evr/dshow: Support NV12 in evr_render
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5157

[PATCH v2 1/1] evr/dshow: Support NV12 in evr_render

[David Gow]

From: David Gow <david(a)ingeniumdigital.com>

Add support for NV12 to evr_render(), as otherwise no video is rendered
at all in games like Age of Empires II DE.

Signed-off-by: David Gow <david(a)ingeniumdigital.com>
---
 dlls/evr/evr.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/dlls/evr/evr.c b/dlls/evr/evr.c
index 516427c5fff..24ee7551c5d 100644
--- a/dlls/evr/evr.c
+++ b/dlls/evr/evr.c
@@ -368,10 +368,21 @@ static HRESULT evr_copy_sample_buffer(struct evr *filter, const GUID *subtype, I
     {
         width = (3 * width + 3) & ~3;
     }
-    else
+    else if (IsEqualGUID(subtype, &MFVideoFormat_NV12))
+    {
+        /* UV plane has same width as Y (half the pixels, twice the components), but half height. */
+        lines = (lines * 3) / 2;
+    }
+    else if (IsEqualGUID(subtype, &MFVideoFormat_ARGB32)
+                || IsEqualGUID(subtype, &MFVideoFormat_RGB32))
     {
         width *= 4;
     }
+    else
+    {
+        ERR("unsupported video format %s\n", debugstr_guid(subtype));
+        return -E_UNEXPECTED;
+    }
 
     if (FAILED(hr = IMediaSample_GetPointer(input_sample, &src)))
     {
@@ -427,7 +438,8 @@ static HRESULT evr_render(struct strmbase_renderer *iface, IMediaSample *input_s
 
     if (IsEqualGUID(&subtype, &MFVideoFormat_ARGB32)
             || IsEqualGUID(&subtype, &MFVideoFormat_RGB32)
-            || IsEqualGUID(&subtype, &MFVideoFormat_YUY2))
+            || IsEqualGUID(&subtype, &MFVideoFormat_YUY2)
+            || IsEqualGUID(&subtype, &MFVideoFormat_NV12))
     {
         if (SUCCEEDED(hr = evr_copy_sample_buffer(filter, &subtype, input_sample, &sample)))
         {
-- 
GitLab

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

[David Gow (＠sulix)]

Turns out I was testing from the wrong build — oops. That explains why the format mysteriously stopped being NV12. Sorry for wasting everyone's time.

When I actually run the correct version, we get NV12-formatted data in `evr_copy_sample_buffer`, and hence just the luma plane, resulting in a wonderful monochromatic green image. Copying the extra plane data then fixes it.

I've pushed a hacky implementation of this (we treat the extra plane as just being more scanlines for the first plane. This works, as they're guaranteed to be the same width / pitch, but I can use a second `MFCopyImage` call instead if that's preferred.

I suspect the _right_ way of doing this involves more explicitly supporting 'multi-plane formats' and looping over them, but a hacky 'if (format == NV12) MFCopyImage(2nd plane)' could work if we don't want to overcomplicate this while there's only one plane.

(I also made the FIXME() an error, which was my original intention with the fallback. I'm happy to just assume we'll never get an unsupported format, though.)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_63036