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.
From: David Gow david@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@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))) {
I don't think I follow. Do you get a newly added fixme for NV12 after your change?
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.
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.
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))- 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.
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.
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
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
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.
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.
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)
@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
-
David Gow -
David Gow (＠sulix)
-
eric pouech (＠epo)
-
Loïc Rebmeister (＠Fox2Code)
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)