From bc9d7000be27f70c4eb9feac1621eda81046d165 Mon Sep 17 00:00:00 2001 From: Andrew Osmond Date: Wed, 13 Feb 2019 14:13:55 -0500 Subject: [PATCH] Bug 1527085 - Ensure we invalidate when the image request changes. r=jrmuizel When the underlying image request (imgIRequest) changes for an image, we need to ensure that we invalidate the cached WebRenderImageData such that the image container stored therein is updated to be for the correct image. This gets a little tricky because some display items store both the current and previous images, and choose to display the latter if the former is not yet ready. We also don't know what image the image container belongs to. As such, we now compare the producer ID of the current frame in the image container, to the expected producer ID of the current image request. If they don't match, we must regenerate the display list. Differential Revision: https://phabricator.services.mozilla.com/D19699 --- gfx/layers/ImageContainer.h | 4 ++-- gfx/layers/ImageTypes.h | 12 ++++++++++++ gfx/layers/ipc/SharedSurfacesChild.cpp | 12 +++++++++++- gfx/layers/ipc/SharedSurfacesChild.h | 3 ++- gfx/layers/wr/WebRenderUserData.cpp | 13 +++++++------ gfx/layers/wr/WebRenderUserData.h | 7 ++++--- image/DynamicImage.cpp | 6 ++++++ image/Image.h | 4 ++++ image/ImageWrapper.cpp | 5 +++++ image/RasterImage.cpp | 8 ++++++++ image/VectorImage.cpp | 9 +++++++++ image/imgIContainer.idl | 5 +++++ image/imgIRequest.idl | 5 +++++ image/imgRequestProxy.cpp | 16 ++++++++++++++++ layout/generic/nsImageFrame.cpp | 4 +++- layout/style/ImageLoader.cpp | 14 ++++++++------ layout/style/ImageLoader.h | 3 ++- layout/xul/nsImageBoxFrame.cpp | 3 ++- 18 files changed, 111 insertions(+), 22 deletions(-) diff --git a/gfx/layers/ImageContainer.h b/gfx/layers/ImageContainer.h index 73a248a24ffb..c16e05c5d109 100644 --- a/gfx/layers/ImageContainer.h +++ b/gfx/layers/ImageContainer.h @@ -387,8 +387,8 @@ class ImageContainer final : public SupportsWeakPtr { */ explicit ImageContainer(const CompositableHandle& aHandle); - typedef uint32_t FrameID; - typedef uint32_t ProducerID; + typedef ContainerFrameID FrameID; + typedef ContainerProducerID ProducerID; RefPtr CreatePlanarYCbCrImage(); diff --git a/gfx/layers/ImageTypes.h b/gfx/layers/ImageTypes.h index a42fb52e20a7..bef9edc03419 100644 --- a/gfx/layers/ImageTypes.h +++ b/gfx/layers/ImageTypes.h @@ -7,6 +7,8 @@ #ifndef GFX_IMAGETYPES_H #define GFX_IMAGETYPES_H +#include // for uint32_t + namespace mozilla { enum class ImageFormat { @@ -106,6 +108,16 @@ enum class YUVColorSpace { UNKNOWN, }; +namespace layers { + +typedef uint32_t ContainerFrameID; +constexpr ContainerFrameID kContainerFrameID_Invalid = 0; + +typedef uint32_t ContainerProducerID; +constexpr ContainerProducerID kContainerProducerID_Invalid = 0; + +} // namespace layers + } // namespace mozilla #endif diff --git a/gfx/layers/ipc/SharedSurfacesChild.cpp b/gfx/layers/ipc/SharedSurfacesChild.cpp index 5f7cdadbaddb..a83e4cd32491 100644 --- a/gfx/layers/ipc/SharedSurfacesChild.cpp +++ b/gfx/layers/ipc/SharedSurfacesChild.cpp @@ -325,7 +325,8 @@ SharedSurfacesChild::AsSourceSurfaceSharedData(SourceSurface* aSurface) { /* static */ nsresult SharedSurfacesChild::Share( ImageContainer* aContainer, RenderRootStateManager* aManager, - wr::IpcResourceUpdateQueue& aResources, wr::ImageKey& aKey) { + wr::IpcResourceUpdateQueue& aResources, wr::ImageKey& aKey, + ContainerProducerID aProducerId) { MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(aContainer); MOZ_ASSERT(aManager); @@ -340,6 +341,15 @@ SharedSurfacesChild::AsSourceSurfaceSharedData(SourceSurface* aSurface) { return NS_ERROR_NOT_AVAILABLE; } + if (aProducerId != kContainerProducerID_Invalid && + images[0].mProducerID != aProducerId) { + // If the producer ID of the surface in the container does not match the + // expected producer ID, then we do not want to proceed with sharing. This + // is useful for when callers are unsure if given container is for the same + // producer / underlying image request. + return NS_ERROR_FAILURE; + } + RefPtr surface = images[0].mImage->GetAsSourceSurface(); if (!surface) { return NS_ERROR_NOT_IMPLEMENTED; diff --git a/gfx/layers/ipc/SharedSurfacesChild.h b/gfx/layers/ipc/SharedSurfacesChild.h index 6d1c734adc3f..e0f8a0ce4cbc 100644 --- a/gfx/layers/ipc/SharedSurfacesChild.h +++ b/gfx/layers/ipc/SharedSurfacesChild.h @@ -15,6 +15,7 @@ #include "mozilla/gfx/UserData.h" // for UserDataKey #include "mozilla/webrender/WebRenderTypes.h" // for wr::ImageKey #include "nsTArray.h" // for AutoTArray +#include "ImageTypes.h" // for ContainerProducerID namespace mozilla { namespace layers { @@ -91,7 +92,7 @@ class SharedSurfacesChild final { static nsresult Share(ImageContainer* aContainer, RenderRootStateManager* aManager, wr::IpcResourceUpdateQueue& aResources, - wr::ImageKey& aKey); + wr::ImageKey& aKey, ContainerProducerID aProducerId); /** * Get the external ID, if any, bound to the shared surface. Used for memory diff --git a/gfx/layers/wr/WebRenderUserData.cpp b/gfx/layers/wr/WebRenderUserData.cpp index ab725277b8fe..3e90af635043 100644 --- a/gfx/layers/wr/WebRenderUserData.cpp +++ b/gfx/layers/wr/WebRenderUserData.cpp @@ -39,7 +39,7 @@ void WebRenderBackgroundData::AddWebRenderCommands( } /* static */ bool WebRenderUserData::ProcessInvalidateForImage( - nsIFrame* aFrame, DisplayItemType aType) { + nsIFrame* aFrame, DisplayItemType aType, ContainerProducerID aProducerId) { MOZ_ASSERT(aFrame); if (!aFrame->HasProperty(WebRenderUserDataProperty::Key())) { @@ -57,7 +57,7 @@ void WebRenderBackgroundData::AddWebRenderCommands( RefPtr image = GetWebRenderUserData(aFrame, type); - if (image && image->UsingSharedSurface()) { + if (image && image->UsingSharedSurface(aProducerId)) { return true; } @@ -107,7 +107,8 @@ WebRenderImageData::~WebRenderImageData() { } } -bool WebRenderImageData::UsingSharedSurface() const { +bool WebRenderImageData::UsingSharedSurface( + ContainerProducerID aProducerId) const { if (!mContainer || !mKey || mOwnsKey) { return false; } @@ -117,7 +118,7 @@ bool WebRenderImageData::UsingSharedSurface() const { // rebuild the scene. wr::ImageKey key; nsresult rv = SharedSurfacesChild::Share( - mContainer, mManager, mManager->AsyncResourceUpdates(), key); + mContainer, mManager, mManager->AsyncResourceUpdates(), key, aProducerId); return NS_SUCCEEDED(rv) && mKey.ref() == key; } @@ -149,8 +150,8 @@ Maybe WebRenderImageData::UpdateImageKey( wr::WrImageKey key; if (!aFallback) { - nsresult rv = - SharedSurfacesChild::Share(aContainer, mManager, aResources, key); + nsresult rv = SharedSurfacesChild::Share(aContainer, mManager, aResources, + key, kContainerProducerID_Invalid); if (NS_SUCCEEDED(rv)) { // Ensure that any previously owned keys are released before replacing. We // don't own this key, the surface itself owns it, so that it can be diff --git a/gfx/layers/wr/WebRenderUserData.h b/gfx/layers/wr/WebRenderUserData.h index bef7c328891b..a01f130ca204 100644 --- a/gfx/layers/wr/WebRenderUserData.h +++ b/gfx/layers/wr/WebRenderUserData.h @@ -13,6 +13,7 @@ #include "mozilla/webrender/WebRenderAPI.h" #include "mozilla/layers/AnimationInfo.h" #include "nsIFrame.h" +#include "ImageTypes.h" class nsDisplayItemGeometry; @@ -57,8 +58,8 @@ class WebRenderUserData { static bool SupportsAsyncUpdate(nsIFrame* aFrame); - static bool ProcessInvalidateForImage(nsIFrame* aFrame, - DisplayItemType aType); + static bool ProcessInvalidateForImage(nsIFrame* aFrame, DisplayItemType aType, + ContainerProducerID aProducerId); NS_INLINE_DECL_REFCOUNTING(WebRenderUserData) @@ -151,7 +152,7 @@ class WebRenderImageData : public WebRenderUserData { bool IsAsync() { return mPipelineId.isSome(); } - bool UsingSharedSurface() const; + bool UsingSharedSurface(ContainerProducerID aProducerId) const; void ClearImageKey(); diff --git a/image/DynamicImage.cpp b/image/DynamicImage.cpp index c7886404bc0d..4de8e96f2446 100644 --- a/image/DynamicImage.cpp +++ b/image/DynamicImage.cpp @@ -120,6 +120,12 @@ DynamicImage::GetType(uint16_t* aType) { return NS_OK; } +NS_IMETHODIMP +DynamicImage::GetProducerId(uint32_t* aId) { + *aId = 0; + return NS_OK; +} + NS_IMETHODIMP DynamicImage::GetAnimated(bool* aAnimated) { *aAnimated = false; diff --git a/image/Image.h b/image/Image.h index 58202b398e2f..7882edd3e2b9 100644 --- a/image/Image.h +++ b/image/Image.h @@ -271,6 +271,10 @@ class ImageResource : public Image { explicit ImageResource(nsIURI* aURI); ~ImageResource(); + layers::ContainerProducerID GetImageProducerId() const { + return mImageProducerID; + } + bool GetSpecTruncatedTo1k(nsCString& aSpec) const; // Shared functionality for implementors of imgIContainer. Every diff --git a/image/ImageWrapper.cpp b/image/ImageWrapper.cpp index f91e3366983f..ee9d26ba398d 100644 --- a/image/ImageWrapper.cpp +++ b/image/ImageWrapper.cpp @@ -131,6 +131,11 @@ ImageWrapper::GetOrientation() { return mInnerImage->GetOrientation(); } NS_IMETHODIMP ImageWrapper::GetType(uint16_t* aType) { return mInnerImage->GetType(aType); } +NS_IMETHODIMP +ImageWrapper::GetProducerId(uint32_t* aId) { + return mInnerImage->GetProducerId(aId); +} + NS_IMETHODIMP ImageWrapper::GetAnimated(bool* aAnimated) { return mInnerImage->GetAnimated(aAnimated); diff --git a/image/RasterImage.cpp b/image/RasterImage.cpp index 0418a8ea0b00..35de98374d20 100644 --- a/image/RasterImage.cpp +++ b/image/RasterImage.cpp @@ -276,6 +276,14 @@ RasterImage::GetType(uint16_t* aType) { return NS_OK; } +NS_IMETHODIMP +RasterImage::GetProducerId(uint32_t* aId) { + NS_ENSURE_ARG_POINTER(aId); + + *aId = ImageResource::GetImageProducerId(); + return NS_OK; +} + LookupResult RasterImage::LookupFrameInternal(const IntSize& aSize, uint32_t aFlags, PlaybackType aPlaybackType, diff --git a/image/VectorImage.cpp b/image/VectorImage.cpp index 0f22effbac5c..77f7f743d907 100644 --- a/image/VectorImage.cpp +++ b/image/VectorImage.cpp @@ -661,6 +661,15 @@ VectorImage::GetType(uint16_t* aType) { return NS_OK; } +//****************************************************************************** +NS_IMETHODIMP +VectorImage::GetProducerId(uint32_t* aId) { + NS_ENSURE_ARG_POINTER(aId); + + *aId = ImageResource::GetImageProducerId(); + return NS_OK; +} + //****************************************************************************** NS_IMETHODIMP VectorImage::GetAnimated(bool* aAnimated) { diff --git a/image/imgIContainer.idl b/image/imgIContainer.idl index 2f66e011c089..7ab8ae63a419 100644 --- a/image/imgIContainer.idl +++ b/image/imgIContainer.idl @@ -142,6 +142,11 @@ interface imgIContainer : nsISupports */ readonly attribute boolean animated; + /** + * Producer ID for image containers created by this image. + */ + [infallible] readonly attribute unsigned long producerId; + /** * Flags for imgIContainer operations. * diff --git a/image/imgIRequest.idl b/image/imgIRequest.idl index ab2e7470904f..25c87176de32 100644 --- a/image/imgIRequest.idl +++ b/image/imgIRequest.idl @@ -29,6 +29,11 @@ interface imgIRequest : nsIRequest */ readonly attribute imgIContainer image; + /** + * Producer ID for image containers created by this image. + */ + [infallible] readonly attribute unsigned long producerId; + /** * Bits set in the return value from imageStatus * @name statusflags diff --git a/image/imgRequestProxy.cpp b/image/imgRequestProxy.cpp index 745eb2e088a2..55d0b077c412 100644 --- a/image/imgRequestProxy.cpp +++ b/image/imgRequestProxy.cpp @@ -10,6 +10,7 @@ #include "imgLoader.h" #include "Image.h" #include "ImageOps.h" +#include "ImageTypes.h" #include "nsError.h" #include "nsCRTGlue.h" #include "imgINotificationObserver.h" @@ -662,6 +663,21 @@ imgRequestProxy::GetImage(imgIContainer** aImage) { return NS_OK; } +NS_IMETHODIMP +imgRequestProxy::GetProducerId(uint32_t* aId) { + NS_ENSURE_TRUE(aId, NS_ERROR_NULL_POINTER); + + nsCOMPtr image; + nsresult rv = GetImage(getter_AddRefs(image)); + if (NS_SUCCEEDED(rv)) { + *aId = image->GetProducerId(); + } else { + *aId = layers::kContainerProducerID_Invalid; + } + + return NS_OK; +} + NS_IMETHODIMP imgRequestProxy::GetImageStatus(uint32_t* aStatus) { if (IsValidating()) { diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp index c1bb9bc45c98..2436caee456d 100644 --- a/layout/generic/nsImageFrame.cpp +++ b/layout/generic/nsImageFrame.cpp @@ -740,7 +740,9 @@ void nsImageFrame::InvalidateSelf(const nsIntRect* aLayerInvalidRect, // Check if WebRender has interacted with this frame. If it has // we need to let it know that things have changed. const auto type = DisplayItemType::TYPE_IMAGE; - if (WebRenderUserData::ProcessInvalidateForImage(this, type)) { + const auto producerId = + mImage ? mImage->GetProducerId() : kContainerProducerID_Invalid; + if (WebRenderUserData::ProcessInvalidateForImage(this, type, producerId)) { return; } diff --git a/layout/style/ImageLoader.cpp b/layout/style/ImageLoader.cpp index 0e15e0fff9c7..dd090d30c8dc 100644 --- a/layout/style/ImageLoader.cpp +++ b/layout/style/ImageLoader.cpp @@ -481,7 +481,7 @@ static bool IsRenderNoImages(uint32_t aDisplayItemKey) { return flags & TYPE_RENDERS_NO_IMAGES; } -static void InvalidateImages(nsIFrame* aFrame) { +static void InvalidateImages(nsIFrame* aFrame, imgIRequest* aRequest) { bool invalidateFrame = false; const SmallPointerArray& array = aFrame->DisplayItemData(); for (uint32_t i = 0; i < array.Length(); i++) { @@ -516,7 +516,7 @@ static void InvalidateImages(nsIFrame* aFrame) { break; case layers::WebRenderUserData::UserDataType::eImage: if (static_cast(data.get()) - ->UsingSharedSurface()) { + ->UsingSharedSurface(aRequest->GetProducerId())) { break; } MOZ_FALLTHROUGH; @@ -532,7 +532,9 @@ static void InvalidateImages(nsIFrame* aFrame) { } } -void ImageLoader::RequestPaintIfNeeded(FrameSet* aFrameSet, bool aForcePaint) { +void ImageLoader::RequestPaintIfNeeded(FrameSet* aFrameSet, + imgIRequest* aRequest, + bool aForcePaint) { NS_ASSERTION(aFrameSet, "Must have a frame set"); NS_ASSERTION(mDocument, "Should have returned earlier!"); @@ -545,7 +547,7 @@ void ImageLoader::RequestPaintIfNeeded(FrameSet* aFrameSet, bool aForcePaint) { // might not find the right display item. frame->InvalidateFrame(); } else { - InvalidateImages(frame); + InvalidateImages(frame, aRequest); // Update ancestor rendering observers (-moz-element etc) nsIFrame* f = frame; @@ -730,7 +732,7 @@ nsresult ImageLoader::OnFrameComplete(imgIRequest* aRequest) { // Since we just finished decoding a frame, we always want to paint, in case // we're now able to paint an image that we couldn't paint before (and hence // that we don't have retained data for). - RequestPaintIfNeeded(frameSet, /* aForcePaint = */ true); + RequestPaintIfNeeded(frameSet, aRequest, /* aForcePaint = */ true); return NS_OK; } @@ -745,7 +747,7 @@ nsresult ImageLoader::OnFrameUpdate(imgIRequest* aRequest) { return NS_OK; } - RequestPaintIfNeeded(frameSet, /* aForcePaint = */ false); + RequestPaintIfNeeded(frameSet, aRequest, /* aForcePaint = */ false); return NS_OK; } diff --git a/layout/style/ImageLoader.h b/layout/style/ImageLoader.h index 1adfc818bf42..36e9cd4627fb 100644 --- a/layout/style/ImageLoader.h +++ b/layout/style/ImageLoader.h @@ -143,7 +143,8 @@ class ImageLoader final : public imgINotificationObserver { nsPresContext* GetPresContext(); - void RequestPaintIfNeeded(FrameSet* aFrameSet, bool aForcePaint); + void RequestPaintIfNeeded(FrameSet* aFrameSet, imgIRequest* aRequest, + bool aForcePaint); void UnblockOnloadIfNeeded(nsIFrame* aFrame, imgIRequest* aRequest); void RequestReflowIfNeeded(FrameSet* aFrameSet, imgIRequest* aRequest); void RequestReflowOnFrame(FrameWithFlags* aFwf, imgIRequest* aRequest); diff --git a/layout/xul/nsImageBoxFrame.cpp b/layout/xul/nsImageBoxFrame.cpp index 6082ca002e4b..96f3a9acad23 100644 --- a/layout/xul/nsImageBoxFrame.cpp +++ b/layout/xul/nsImageBoxFrame.cpp @@ -824,7 +824,8 @@ nsresult nsImageBoxFrame::OnFrameUpdate(imgIRequest* aRequest) { // Check if WebRender has interacted with this frame. If it has // we need to let it know that things have changed. const auto type = DisplayItemType::TYPE_XUL_IMAGE; - if (WebRenderUserData::ProcessInvalidateForImage(this, type)) { + const auto producerId = aRequest->GetProducerId(); + if (WebRenderUserData::ProcessInvalidateForImage(this, type, producerId)) { return NS_OK; }