Possible Bug in ReactImage

[ohnnyj]

Stumbled upon the following code in ReactImage.cpp
https://github.com/microsoft/react-native-windows/blob/main/vnext/Microsoft.ReactNative/Views/Image/ReactImage.cpp#L432C1-L442C12

          bitmapImage.UriSource(uri);

          // It is possible that the same URI will be set twice if an intermediate update occurs that requires
          // asynchronous behavior (e.g., when the ImageSourceType is ::Download or ::InlineData). In this case,
          // do not set the opacity to zero as the ImageOpened event will not fire.
          auto currentUri = bitmapImage.UriSource();
          if (uri && currentUri && uri.AbsoluteUri() != currentUri.AbsoluteUri()) {
            // TODO: When we change the source of a BitmapImage, we're getting a flicker of the old image
            // being resized to the size of the new image. This is a temporary workaround.
            imageBrush.Opacity(0);
          }

Maybe I am reading this wrong but it looks like the if condition will always fail. The curentUri is being read after setting it to uri. Wouldn't this make them the same. I would expect auto currentUri = bitmapImage.UriSource(); to be moved just above setting the new value.

[chrisglein]

The code was added specifically as part of this commit: 0329f9b
The rationale is there, hopefully it makes sense.

[ohnnyj]

Thanks @chrisglein.
I understand the rationale for the change in the linked commit but think the code change introduced a different issue.
The image brush opacity will now never be set to zero which effectively does fix the linked issue but will no longer prevent the flash mentioned in the comment.

[chrisglein]

It certainly looks odd.

 bitmapImage.UriSource(uri);

auto currentUri = bitmapImage.UriSource();

After that it seems like uri and currentUri should be identical.

So then this should always be false:

uri.AbsoluteUri() != currentUri.AbsoluteUri()

Transparently, we're unlikely to fix this as our energy is focused on the new Fabric renderer, where this entire class is replaced. And we have a suspected problem, but not a clear impact. Interesting find though - how did you stumble across it?

@rozele Do you remember the details of your change linked above?

[rozele]

I think it should be fine to just read the currentUri value before setting uri. I think this was just an oversight / failure to test the opacity 0 functionality.

[ohnnyj]

Transparently, we're unlikely to fix this as our energy is focused on the new Fabric renderer, where this entire class is replaced. And we have a suspected problem, but not a clear impact. Interesting find though - how did you stumble across it?

Completely understand. Not a big deal. Just stumbled upon it when looking at some SVG issues (#11988, #11989).