nits : let's print the value of isHDR in logging.

We can remove aFallbackSubType here as it's not used at all.

Update the comment for functions as well, same for below.

Same here, if HDR is not supported in zero copy, we should add a comment to explicitly explain why it can't be supported.

Print mIsHDR as well.

This comment seems implying that HDR has the lowest priority, which is wrong. Put HDR above if HW....

Align these three FAILED(...).

As ConfigureForSize will set the output type to MFVideoFormat_P010 when mIsHDR is true , but if the decoder output type is set to MFVideoFormat_NV12/MFVideoFormat_YV12 or even MFVideoFormat_P016, what will happen when those two types are unmatched?

Maybe we should instead let ConfigureForSize knows what format it should set?

Short answer - no. I was going off of the comments text here and I honestly don't know what this does, only that I encountered it when debugging. I'll see if it works the same without this change.

Seems to work the same so I have removed this change.

I tried to make zerocopy work but I am not totally sure I understand how it functions, it's worth noting that dcomp can't display DXGI_FORMAT_P010 with DXGI_COLOR_SPACE_YCBCR_STUDIO_G2084_LEFT_P2020, so if the 'copy' refers to converting from P010 to RGB10A2 then we need the copy, if the copy is some other part of the processing then we may not, I can say that the zerocopy code has an awful lot of mentions of NV12 in it so it looked possibly non-trivial to upgrade.

It's worth noting that I tried requesting decode to subtype MFVideoFormat_A2B10G10R10 and it could not find find a matching decoder, at least on AMD. I haven't tried other vendors to confirm if others might be able to skip a step.

I've found that I no longer use aFallbackSubType in MFTDecoder::Create so I've removed this change.

SetMediaTypes takes a aFallbackSubType, which implements the previously internal fallback to MFAudioFormat_Float in this case, so no change in behavior but this is now controlled by the caller.

I'm not completely certain that it can't, it depends what zerocopy is actually trying to do - if it's skipping a copy of a processing buffer then that probably can work, but if it's skipping the MFVideoFormat_P010->MFVideoFormat_A2B10G10R10 conversion (which is necessary) then it can't support HDR.

I don't follow? I was preserving the existing behavior, which always tried NV12/YV12 first, it never even attempted P010 or P016 because of the behavior of MFTDecoder::SetDecoderOutputType.

Refactored to actually shorten the logic now that I made GUID_NULL a valid thing to pass as aFallbackSubType, so that this logic can actually work for the first time - previously before my changes in this patch only the first one mattered since it would never fail as all fallback logic is inside MFTDecoder::SetDecoderOutputType.

I assume this is resolved then.

Okay, I've reduced it to that subset and added a gfxCriticalNote to indicate if this is reached.

I will be doing that minor refactor soon.

It's usually better to not uselessly duplicate state when it can be a method.

bool IsHDR() const {
  gfx::TransferFunction transferFunction =
    mVideoInfo.mTransferFunction.refOr(gfx::TransferFunction::BT709);
  return (transferFunction == gfx::TransferFunction::PQ ||
              transferFunction == gfx::TransferFunction::HLG);
}

@sotaro could you help us clarify the zero copy a little bit? will it be able to support HDR? Thanks!

Sorry for not being clear earlier. I just meant that the new comment form (if XXX...) is a bit confusing, as it gives me an impression that we would check HW first, then SW, and HDR last.

Maybe rephrase it as: Note: we will check P010/P016 first if HDR is preferred.

This is still not addressed yet.

I've moved this logic to a static inline function in gfx/2d/Types.h called IsHDRTransferFunction, and also added a convenience method WMFVideoMFTManager::IsHDR to avoid duplicating the rather verbose call of IsHDRTransferFunction(mVideoInfo.mTransferFunction.refOr(gfx::TransferFunction::BT709)) where it cares about that.

This also could be called in DCLayerTree once the mTransferFunction is plumbed through the TextureHost stuff in future, which would let us decide if it is an HDR texture rather than speculating that BT2020 is always HDR (which is not true from a spec standpoint but from a videos-found-in-the-wild perspective seems to be universally true as we've been shipping that assumption for macOS HDR video for some time now).

I hope that resolves the concern?

Okay, I've replaced the comment with a more detailed explanation of rationale with HDR vs SDR as the top level decision for the two fallback sequences, and also added an explanation for the use of GUID_NULL and what that does in MFTDecoder::SetDecoderOutputType.

I'm going to think a bit about how to address this. Thanks for highlighting it.

My understanding of the video pipeline being pretty vague here - I've written video codecs from scratch before and I understand the multi-stage transforms that typically occur in a video decode pipeline, but I'm not sure what parts of the pipeline each of these choices represents in context of WMF and DVXA2, I've been assuming the choice here is picking what the stream decoder will output (so typically a YUV format), and the choice in ConfigureForSize is the target YUV/RGB format to blit that into when we wrap it in TextureHost to send on to DCLayerTree to put in a compositor swapchain.

Per discussion with Sotaro on Matrix, yes zerocopy can work, it involves passing around the decoder output buffer rather than making an internal copy to pass to the color transform, so I'll look into making it work, the code path looks non-trivial to understand and it has an obsession with NV12 so it will take a bit to rework that.

I'd prefer to put zerocopy in as a separate patch because it's not necessary for the feature to work, but does improve performance., so if it causes regressions I'd prefer it be rolled back separately.

Per discussion with Sotaro, zerocopy is just avoiding an unnecessary copy of a processing buffer on the GPU, so it definitely can support HDR. However zerocopy is currently coded in a way that only works with NV12 and I'm not sure refactoring it should be in scope for this patch. So I am focusing on adding relevant comments to indicate it could be done but isn't yet.

After looking over this carefully, I removed the subtype override in ConfigureForSize, and changed the output format choices to RGB10A2 and BGRA8. I verified that there's no situation where the subtype coming into ConfigureForSize mismatches the incoming video type.