Add HDR tone map and white level adjustment effects

[benstevens48]

This is my initial attempt to add the HDR tone map and white level adjustment effects. The main question is how to do the is effect supported check. What I've done is done a check that tries to create the effect and caches the result. It actually takes a key so that the cached result can apply to more than one effect. I check for an E_NOT_SET result to indicate the effect is missing. According to my tests this is the result returned if there is no such effect. I'd appreciate some feedback from @shawnhar as to whether this is the correct thing to do or whether the approach is good in general, especially as I will be using this in my own project anyway regardless of whether this pull request gets accepted.

The other minor thing I was unsure about is the file UpdateApiResourceFiles.cmd. I think I probably should have added the new effects to the FILE_LIST in here, but I didn't really understand what this file did and have no way of verifying it.

[shawnhar]

Thanks for contributing this! It looks really good, very close to being ready to merge.

I don't think it's the right approach to try to create an effect in order to determine whether that type is supported. Actually creating the effect is quite heavyweight (hence the need for a cache) but the implementation could be much simpler if this cache was not needed.

There's an IsEffectRegistered helper in PixelShaderEffectImpl.cpp that I believe could be moved somewhere more general purpose and used for this task. My only slight doubt is whether this mechanism will report the built-in effect GUIDs as well as custom registered ones, but from reading the D2D docs I'm 90% sure it does.

UpdateApiResourceFiles.cmd is a bit unclean :-) Those XML files which describe the image effects are an implementation detail of the Direct2D layer underneath Win2D. We decided to use them as inputs to the Win2D effect codegen tool, so published copies of all the relevant D2D .xml files as part of Win2D. This batch file copies across the latest versions from a D2D source enlistment (which non-MSFT folks of course do not have).

It looks like you've figured out the format of that XML pretty well to create new ones of your own, but I can provide the exact two latest ones from D2D if that would be useful?

[shawnhar]

Super nitpicky comment: I suspect the SDK requirements (in README.md, "Building Win2D from source" section) will need updating as a result of this feature addition?

[benstevens48]

Hi Shawn,
Having those two extra files would be useful just in case there are any min/max values that I've missed. I've also just updated UpdateApiResourceFiles.cmd to include the extra two xml files in the list since I assume your offer to provide them means they do actually exist.

I just did some very rough performance testing, and according to that, actually creating the effect (or failing to create it) is significantly faster than returning the list of registered effects (most of the time). I found the performance was very inconsistent, but returning the list of registered effects can be quite slow the first time it is run (even if some other D2D stuff has run first). I did a test where I tried the create effect method followed by the check if registered method. The create effect took at most 1ms and was more typically 0.05ms or quicker, whereas getting the list of registered effects took up to 200ms on the first try (although sometimes only 2ms), then more typically 0.1ms on subsequent tries. So I'm wondering whether to leave the test as it is? Or could it be the case that some effects are more expensive to create than others? In that case I could hard-code the test for these two to use the white level adjustment effect or something. The other downside of my method is that it requires a device I suppose, but ultimately you are always going to require one so that doesn't seem too bad. What do you think? (Also, is checking E_NOT_SET to determine whether the effect is present using my method safe?). And yes, getting the list of registered effects does return built-in ones (and intellisense tells me this although I couldn't find it documented elsewhere).

It seems that the SDK version in the readme was updated a white ago to 18362 whereas this update only requires 17763.

[shawnhar]

I'm not worried about the first time perf overhead, as this is a runtime init cost that any app using effects will pay in any case the first time it tries to render with them. As long as that cost isn't repeated on every call to the property getter, it seems fine to me.

Requiring a device behind the scenes for an API that doesn't take one as a parameter is a problem, though. One issue there is which device to pick, if there is more than one GPU in the system? Best option is to just use the default GPU, but what if the app is running on the other one? That could result in a VERY expensive operation, loading user mode drivers for a GPU that wasn't previously in use, spinning the hardware up out of power saving sleep mode, etc. Quite surprising if an IsSupported property getter could end up triggering that much work :-)

Another issue is what if the device becomes lost while the test is being made? Perhaps unrelated rendering code triggered a hardware fault, or the graphics driver is in the process of being upgraded. Code that knows it is using a device has the option to handle the device lost state and recover gracefully, but there is no indication to the caller that an IsSupported getter might have that kind of dependency, and no way for a caller to recover if it did encounter that kind of error (they can't just recreate the device and try again, because they don't even know which device was being used internally).

So I think it's important to use the technique that does not require a device here. Hopefully that can be done just for the two new effects, leaving the previous set which also have IsSupported properties checking this via the older QueryInterface method instead.

Good call on the SDK version in readme - you're quite right!

[benstevens48]

I wasn't proposing to create a device for the IsSupported check - the the current implementation in the pull request takes a CanvasDevice as a parameter in the same way that CanvasImage.IsHistogramSupported does, so there's no overhead there.

Anyway, I'll try and create a version which relies on checking the registered effects tomorrow, if you think that's best. The occasionally very high initial cost of the check does worry me slightly though, since it may be performed on the main UI thread. The thing is that it seems to be higher than creating a single effect for the first time, so it doesn't seem an inevitable cost.

[shawnhar]

I pushed copies of the official metadata for the two new effects to https://github.com/shawnhar/Win2D/tree/new_effect_xml, if you want to grab them from there. I think the result from the codegen tool will be equivalent to the ones you created manually, but as you say it can't hurt to use the official versions to make sure there isn't some subtle difference that we are missing.

[benstevens48]

Thanks - annoyingly it looks like HDR and SDR in these XML files don't conform to C# naming conventions - should I try to override these using Settings.xml or something?

Also, just a thought before I do any work - is it worth trying to use GetEffectProperties instead for the IsSupported check? I can investigate to see if this is quicker.

[shawnhar]

annoyingly it looks like HDR and SDR in these XML files don't conform to C# naming conventions - should I try to override these using Settings.xml or something?

Yep, that's exactly what settings.xml is for: you'll see many name overrides for the previous effects. The idea is to base the Win2D effect codegen off these official D2D effect definitions, so that things like min/max values and the full set of parameters are guaranteed correct, but override names wherever needed to fit WinRT (which is the same as .NET) style guidelines.

Using GetEffectProperties sounds like a good idea to me. I doubt that this will make a big performance difference in practice, but that would be clean and simple, and avoids taking a dependency on anything other than the existence of the effect being queried.

[benstevens48]

Hi Shawn,

Ok, I just made all the updates that I think were needed. Hopefully it's now done in a way that can be extended easily in the future. GetEffectProperties seemed to be pretty fast.

A couple of points to clarify. Technically, the IsSupported check can throw an exception in rare cases (e.g. insufficient memory, but it would need to be very low...). Is it worth warning about this in the docs? The existing IsSupported check can technically throw an exception as well if it fails to create the factory and there is no warning for that. Also, I added the check to the effect constructor in the same way as the existing IsSupported checks, so the same applies there.

A final minor point - I marked these new effects as NoComposition in the codes. I don't know whether this is true or not but I thought it was better to err on this side of caution.

[benstevens48]

Actually, another final point. At the moment after GetEffectProperties I throw if the result is not D2DERR_EFFECT_IS_NOT_REGISTERED or E_NOT_SET. Do you think this is 100% reliable? Or should we treat all errors as a negative result?

[shawnhar]

This is looking really good! I've cloned it locally and am running some sanity tests now (all seems good so far...)

Technically, the IsSupported check can throw an exception in rare cases (e.g. insufficient memory, but it would need to be very low...). Is it worth warning about this in the docs?

No, because these rare cases are not a meaningful error that an app could handle and recover from. Eric Lippert wrote a nice summary of different categories of exception: https://docs.microsoft.com/en-us/archive/blogs/ericlippert/vexing-exceptions. Using his terminology, things like insufficient memory or failure to create a D2D factory are "Fatal" exceptions, while GPU device removal is an "Exogenous" exception.

A final minor point - I marked these new effects as NoComposition in the codes. I don't know whether this is true or not but I thought it was better to err on this side of caution.

Correct, composition does not (yet?) support these effects.

At the moment after GetEffectProperties I throw if the result is not D2DERR_EFFECT_IS_NOT_REGISTERED or E_NOT_SET. Do you think this is 100% reliable? Or should we treat all errors as a negative result?

Hmm, good question. Normally I would say its best to be specific and look for only the exact error code that we're interested in, but this particular case we already have to handle multiple results due to the bizarre behavior where UWP is reporting E_NOT_SET. This has me concerned that other execution environments (including perhaps ones that get created in the future, which of course we have no way to test on today) might return yet another different error code here. It might be safer to just set supported = SUCCEEDED(hr). Worst case, this could result in reporting effect not supported if the system is in a badly broken state for some other reason, but that seems harmless to me. Technically, the effect is indeed not supported if the system is broken, right? :-) And if GetEffectProperties did fail for some unexpected reason, surely any subsequent attempt to use the effect will then also fail, at which point a proper exception would be thrown.

One thought: is the m_cachedRegisteredEffects system really needed? I don't feel strongly about this, but wonder if the perf gain is really worth that added complexity and need for tracking persistent state (in particular the need to lock around querying and updating the shared dictionary).

Optional suggestion: consider adding the two new effects to the Example Gallery sample/test app? That contains an effects demo which allows selecting any of the image effects and shows them in (usually fairly simple) visual demo. I won't block completing the PR on not including this if you don't want to bother, just thought I'd point it out.

[shawnhar]

My smoke test all came back clean, so I'm ready to merge this whenever you give me the go-ahead. Holding off for now in case you decide to make any final tweaks based on our last couple of messages.

[benstevens48]

I've changed the supported check to just test for SUCCEEDED(hr) as you suggested. I don't understand why/how the error codes are different in UWP, but this does seem safest.

I think the cache is probably worth it. The old is supported check has a cache and that's doing less work because it doesn't have to call GetEffectProperties.

I agree it would be nice to update the example gallery. However, I'm doing most of this in my spare time so it's probably best if I don't spend too much more time on it for now. (Also there's less incentive for me to update that since it's of no use in my own app). I have tested that the effects work though.

I'm happy for this pull request to be merged.

[shawnhar]

Looks good to me. Many thanks for the great contribution!

[zhuman]

@shawnhar It looks like this PR is not in the "winappsdk/main" branch? It seems that "master" turned into "uwp/main" but this was never merged into the new main which is WinUI3-based.