[PATCH 0/1] MR5157: evr/dshow: List NV12 as supported, improve warnings
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157
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. 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. Signed-off-by: David Gow <david(a)ingeniumdigital.com> --- dlls/evr/evr.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/dlls/evr/evr.c b/dlls/evr/evr.c index 516427c5fff..53085f32ef0 100644 --- a/dlls/evr/evr.c +++ b/dlls/evr/evr.c @@ -368,10 +368,15 @@ static HRESULT evr_copy_sample_buffer(struct evr *filter, const GUID *subtype, I { width = (3 * width + 3) & ~3; } - else + else if (IsEqualGUID(subtype, &MFVideoFormat_ARGB32) + || IsEqualGUID(subtype, &MFVideoFormat_RGB32)) { width *= 4; } + else + { + FIXME("unsupported video format %s\n", debugstr_guid(subtype)); + } if (FAILED(hr = IMediaSample_GetPointer(input_sample, &src))) { @@ -427,7 +432,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
I don't think I follow. Do you get a newly added fixme for NV12 after your change? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62615
On Mon Feb 26 10:29:25 2024 +0000, Nikolay Sivov wrote:
I don't think I follow. Do you get a newly added fixme for NV12 after your change? No — that's the bit I don't understand. With this patch, the video renders correctly, but there's no fixme. The RGB32 codepath is taken in ``evr_copy_sample_buffer()`` instead.
I put the fixme there in case this doesn't happen in all circumstances, but have never hit it personally. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62620
On Mon Feb 26 10:29:25 2024 +0000, David Gow wrote:
No — that's the bit I don't understand. With this patch, the video renders correctly, but there's no fixme. The RGB32 codepath is taken in ``evr_copy_sample_buffer()`` instead. I put the fixme there in case this doesn't happen in all circumstances, but have never hit it personally. Yeah, that doesn't make any sense. I'd rather understand what's actually going on here first.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62692
Loïc Rebmeister (@Fox2Code) commented about dlls/evr/evr.c:
{ width = (3 * width + 3) & ~3; } - else + else if (IsEqualGUID(subtype, &MFVideoFormat_ARGB32) + || IsEqualGUID(subtype, &MFVideoFormat_RGB32)) { width *= 4; } + else + { + FIXME("unsupported video format %s\n", debugstr_guid(subtype));
This change behavior and might introduce regression due to `width *= 4;` not being called. Adding `width *= 4;` as a default assumption like before is probably better. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62815
On Tue Feb 27 14:31:26 2024 +0000, Loïc Rebmeister wrote:
This change behavior and might introduce regression due to `width *= 4;` not being called. Adding `width *= 4;` as a default assumption like before is probably better. I don't think that makes sense. BGRA and BGRx are the only real raw 32-bit raw formats.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62833
On Tue Feb 27 17:03:29 2024 +0000, Zebediah Figura wrote:
I don't think that makes sense. BGRA and BGRx are the only real raw 32-bit raw formats. note also that in NV12 case you now copy 32bpp, whereas NV12 is only 12bpp
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62926
On Wed Feb 28 13:51:03 2024 +0000, eric pouech wrote:
note also that in NV12 case you now copy 32bpp, whereas NV12 is only 12bpp but also YUY2 is 16bpp, and we currently (preexisting to this MR) copying 24bpp unless I'm mistaken
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_62930
On Wed Feb 28 14:00:32 2024 +0000, eric pouech wrote:
but also YUY2 is 16bpp, and we currently (preexisting to this MR) copying 24bpp unless I'm mistaken I was talking about the fallback which doesn't necessarily needs to have very precise default. But both of you seems to have more deeper knowledge about the code change in question.
I'll wish you both suggested code changes directly, doesn't need to be anything fancy, can be just a small markdown code block like I did. Since 12bpp is not a multiple of 8, I think `width = (width * 3 + 1) / 2` should be correct for NV12? Maybe if it needs to be 4 bytes aligned buffer size it would be `width = ((width * 3 + 1) / 2 + 3) & ~3` instead. Personally, I'll go for the 4 byte aligned 12bpp to be safe, and add a comment explaining it's 4 byte aligned 12bpp. But I'm not perfectly familiar with this part of wine yet, so any feedback from someone more knowledgeable would be welcome. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_63010
On Thu Feb 29 00:29:34 2024 +0000, Loïc Rebmeister wrote:
I was talking about the fallback which doesn't necessarily needs to have very precise default. But both of you seems to have more deeper knowledge about the code change in question. I'll wish you both suggested code changes directly, doesn't need to be anything fancy, can be just a small markdown code block like I did. Since 12bpp is not a multiple of 8, I think `width = (width * 3 + 1) / 2` should be correct for NV12? Maybe if it needs to be 4 bytes aligned buffer size it would be `width = ((width * 3 + 1) / 2 + 3) & ~3` instead. Personally, I'll go for the 4 byte aligned 12bpp to be safe, and add a comment explaining it's 4 byte aligned 12bpp. But I'm not perfectly familiar with this part of wine yet, so any feedback from someone more knowledgeable would be welcome. I don't think we want a fallback.
We really shouldn't even be succeeding connection for any format we're not going to handle. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_63020
On Thu Feb 29 03:26:06 2024 +0000, Zebediah Figura wrote:
I don't think we want a fallback. We really shouldn't even be succeeding connection for any format we're not going to handle. @zfigura are you saying this about adding NV12 as supported format, or about trying to mess up with a fallback (I understand the later, just want to be sure)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_63029
@Fox2Code YUY2 is a packed format (ie a single plane with pack YUV info), with UV sampling on two consecutive (horizontal) pixels. hence width should be forced even and the width for the copy should be recomputed as ((width + 1) & \~1) \* 2 regarding NV12, there are two planes; one with 8bpp, the second (following the first) with 4bpp. this should require two calls to MFCopyImage for each plane but I was hoping that @gow would follow up on this -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5157#note_63030
participants (6)
-
David Gow -
David Gow (＠sulix)
-
eric pouech (＠epo)
-
Loïc Rebmeister
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)