Closed Bug 1376855 Opened 2 years ago Closed 2 years ago

Support display items that use image for layer free mode

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ethlin, Assigned: mtseng)

References

Details

Attachments

(7 files)

We need to retain image key, image container and image client for image type items. And we should use the same mechanism for both layer/non-layer paths so that we could remove the old interface of nsDisplayItem::CreateWebRenderCommands.
Summary: Support image type display items for layer free mode → Support display items that use image for layer free mode
Comment on attachment 8882257 [details]
Bug 1376855 - Introduce WebRenderUserData and WebRenderImageData.

https://reviewboard.mozilla.org/r/153356/#review158614

Minusing for now because of the first comment below (using the nsIFrame pointer like this is not really safe). I suspect this patch will change somewhat significantly after taking that into account, so I'll re-review once it's updated.

::: gfx/layers/wr/WebRenderLayerManager.h:30
(Diff revision 1)
>  class KnowsCompositor;
>  class PCompositorBridgeChild;
>  class WebRenderBridgeChild;
>  class WebRenderParentCommand;
>  
> +typedef Pair<nsIFrame*, uint32_t> FrameDisplayItemKey;

I don't think you can do this safely. I checked with :mstange as well - because of frame reconstruction, an nsIFrame can be destroyed and another nsIFrame can be allocated in its place. That would leave you with a dangling pointer, or a pointer to the wrong iframe, or something along those lines.

Unless you strictly control the lifetime of these keys and throw them away on frame reconstruction, this is likely to run into problems.

:mstange suggested a better approach might be to use the frame property table instead if you want to associate things with frames: http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/layout/generic/nsIFrame.h#3497,3504,3511

::: gfx/layers/wr/WebRenderLayerManager.h:33
(Diff revision 1)
> + * A hash entry that combines frame pointer and display item's per frame key.
> + */
> +class FrameDisplayItemKeyHashEntry : public PLDHashEntryHdr

Please move these auxiliary classes out into their own files instead of bloating WebRenderLayerManager.h

::: gfx/layers/wr/WebRenderLayerManager.h:80
(Diff revision 1)
> +    : mWRManager(aWRManager)
> +  { }
> +
> +  virtual WebRenderImageData* AsImageData() { return nullptr; }
> +
> +  enum class TYPE {

s/TYPE/Type/

or maybe UserDataType or DataType or something less ambiguous. Please use the eXxx naming convention for enum members, so eImage instead of IMAGE.
Attachment #8882257 - Flags: review?(bugmail) → review-
Blocks: layers-free
Sorry for the delay here, I don't know some of this code as well as I should so it's taking me some time to properly review.
Blocks: 1377571
Comment on attachment 8882258 [details]
Bug 1376855 - Support nsDisplayBackgroundImage in layers free mode.

https://reviewboard.mozilla.org/r/153358/#review159308
Attachment #8882258 - Flags: review?(bugmail) → review+
Comment on attachment 8882257 [details]
Bug 1376855 - Introduce WebRenderUserData and WebRenderImageData.

https://reviewboard.mozilla.org/r/153356/#review159304

::: gfx/layers/wr/WebRenderUserData.cpp:139
(Diff revision 2)
> +
> +void
> +WebRenderImageData::CreateExternalImageIfNeeded()
> +{
> +  if (!mExternalImageId)  {
> +    MOZ_ASSERT(mImageClient);

In CreateImageClientIfNeeded you seem to gracefully handle the case where mImageClient is null. Why assert that it's non-null here then? If you need it to be non-null shouldn't that assert go into CreateImageClientIfNeeded()? This function is called directly after CreateImageClientIfNeeded from UpdateImageKey and is the only thing that assigns to mExternalImageId so the fact that this assertion is inside a !mExternalImageId condition is irrelevant.

(Upon further examination it looks like you lifted this code out of WebRenderDisplayItemLayer::SendImageContainer, but there it was all in one function so the assert made sense. Here it doesn't)
Attachment #8882257 - Flags: review?(bugmail) → review+
Comment on attachment 8882259 [details]
Bug 1376855 - Support nsDisplayVideo in layers free mode.

https://reviewboard.mozilla.org/r/153360/#review159310

This looks ok to me but I don't really know this code. I'll flag sotaro also for review on this.

::: layout/generic/nsVideoFrame.cpp:459
(Diff revision 2)
> +      // No image, or zero-sized image. No point creating a layer.
> +      return false;

Since there's nothing to do here, should we just return true? Otherwise we will try to use the fallback mechanism for no reason.
Attachment #8882259 - Flags: review?(bugmail) → review+
Comment on attachment 8882260 [details]
Bug 1376855 - Support nsDisplayImage in layers free mode.

https://reviewboard.mozilla.org/r/153362/#review159312
Attachment #8882260 - Flags: review?(bugmail) → review+
Comment on attachment 8882261 [details]
Bug 1376855 - Support nsDisplayBullet in layers free mode.

https://reviewboard.mozilla.org/r/153364/#review159314

::: layout/generic/nsBulletFrame.cpp:676
(Diff revision 2)
>  
>  void nsDisplayBullet::Paint(nsDisplayListBuilder* aBuilder,
>                              gfxContext* aCtx)
>  {
>    uint32_t flags = imgIContainer::FLAG_NONE;
> -  if (aBuilder->ShouldSyncDecodeImages()) {
> +  if (aBuilder && aBuilder->ShouldSyncDecodeImages()) {

Why would aBuilder ever be null here?
Attachment #8882261 - Flags: review?(bugmail) → review+
Comment on attachment 8882262 [details]
Bug 1376855 - Support image mode of nsDisplayBorder in layers free mode.

https://reviewboard.mozilla.org/r/153366/#review159316
Attachment #8882262 - Flags: review?(bugmail) → review+
Comment on attachment 8882263 [details]
Bug 1376855 - Removed unused variables and interfaces.

https://reviewboard.mozilla.org/r/153368/#review159318
Attachment #8882263 - Flags: review?(bugmail) → review+
Comment on attachment 8882261 [details]
Bug 1376855 - Support nsDisplayBullet in layers free mode.

https://reviewboard.mozilla.org/r/153364/#review159432

::: layout/generic/nsBulletFrame.cpp:676
(Diff revision 2)
>  
>  void nsDisplayBullet::Paint(nsDisplayListBuilder* aBuilder,
>                              gfxContext* aCtx)
>  {
>    uint32_t flags = imgIContainer::FLAG_NONE;
> -  if (aBuilder->ShouldSyncDecodeImages()) {
> +  if (aBuilder && aBuilder->ShouldSyncDecodeImages()) {

I accidently added this. Will remove it.
Comment on attachment 8882259 [details]
Bug 1376855 - Support nsDisplayVideo in layers free mode.

https://reviewboard.mozilla.org/r/153360/#review159430

::: layout/generic/nsVideoFrame.cpp:459
(Diff revision 2)
> +      // No image, or zero-sized image. No point creating a layer.
> +      return false;

hmm, return true makes more sense. I'll return true in newer patch!
Comment on attachment 8882257 [details]
Bug 1376855 - Introduce WebRenderUserData and WebRenderImageData.

https://reviewboard.mozilla.org/r/153356/#review159428

::: gfx/layers/wr/WebRenderUserData.cpp:139
(Diff revision 2)
> +
> +void
> +WebRenderImageData::CreateExternalImageIfNeeded()
> +{
> +  if (!mExternalImageId)  {
> +    MOZ_ASSERT(mImageClient);

You're right. I think we shouldn't assert here. I removed it.
Comment on attachment 8882259 [details]
Bug 1376855 - Support nsDisplayVideo in layers free mode.

https://reviewboard.mozilla.org/r/153360/#review159482
Attachment #8882259 - Flags: review?(sotaro.ikeda.g) → review+
Thanks for the review. Try looks fine.
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47c66f214de3
Introduce WebRenderUserData and WebRenderImageData. r=kats
https://hg.mozilla.org/integration/autoland/rev/48c63b6fe97f
Support nsDisplayBackgroundImage in layers free mode. r=kats
https://hg.mozilla.org/integration/autoland/rev/084ec6e40c1a
Support nsDisplayVideo in layers free mode. r=kats,sotaro
https://hg.mozilla.org/integration/autoland/rev/6727afa9153d
Support nsDisplayImage in layers free mode. r=kats
https://hg.mozilla.org/integration/autoland/rev/c14a46dee9f0
Support nsDisplayBullet in layers free mode. r=kats
https://hg.mozilla.org/integration/autoland/rev/84897fa3a8fb
Support image mode of nsDisplayBorder in layers free mode. r=kats
https://hg.mozilla.org/integration/autoland/rev/19c0cc2980de
Removed unused variables and interfaces. r=kats
Depends on: 1379120
You need to log in before you can comment on or make changes to this bug.