If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Change CanvasImageCache to look cache images based on imgIContainer instead of imgIRequest

RESOLVED FIXED in Firefox 49

Status

()

Core
Canvas: 2D
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

({perf})

44 Branch
mozilla49
All
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
Currently the CanvasImageCache caches drawImage canvas sources based on imgIRequest and a combination of the source element and canvas element. This caused cache misses in bug 1250073 as two images with the same source but on different <img> elements would miss the cache. Using imgIContainer, we can optimize this so that drawImage sources that are the same image across canvases or elements would still be a cache hit. 

The wrinkle here is that due to CORS security, we actually need two caches. One for drawImage sources on the same canvas element and one for drawImage sources on different canvas elements.
(Assignee)

Comment 1

a year ago
Created attachment 8744912 [details] [diff] [review]
Change CanvasImageCache to look cache images based on imgIContainer instead of imgIRequest
Attachment #8744912 - Flags: review?(seth)
(Assignee)

Comment 2

a year ago
Try looking good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae2ff8d7a19e
(Assignee)

Comment 3

a year ago
Comment on attachment 8744912 [details] [diff] [review]
Change CanvasImageCache to look cache images based on imgIContainer instead of imgIRequest

Review of attachment 8744912 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4418,5 @@
> +    return res;
> +  }
> +
> +  res.mSourceSurface =
> +    CanvasImageCache::LookupAllCanvas(aElement, mIsSkiaGL);

Just an FYI, this is the same as the deleted function except this one line.
See Also: → bug 1250073
Comment on attachment 8744912 [details] [diff] [review]
Change CanvasImageCache to look cache images based on imgIContainer instead of imgIRequest

Review of attachment 8744912 [details] [diff] [review]:
-----------------------------------------------------------------

Love it. A lot of style nits, but an enthusiastic r+.

::: dom/canvas/CanvasImageCache.cpp
@@ +36,5 @@
>    HTMLCanvasElement* mCanvas;
>    bool mIsAccelerated;
>  };
>  
> +/***

Three stars? Madness!

@@ +38,5 @@
>  };
>  
> +/***
> + * Cache data needs to be separate from the entry
> + * for nsExpirationTracker

Putting a period at the end of the sentences in your comments would make me very happy.

@@ +97,5 @@
>  };
>  
> +
> +/***
> + * Used for all images across all canvases

See above.

@@ +99,5 @@
> +
> +/***
> + * Used for all images across all canvases
> + */
> +struct AllCanvasImageCacheKey {

Much better name. Since you're touching this line, considering moving the '{' to the next line to comply with our formatting standards.

@@ +111,4 @@
>    bool mIsAccelerated;
>  };
>  
> +class AllCanvasImageCacheEntry : public PLDHashEntryHdr {

Same here about '{'.

@@ +121,4 @@
>      , mIsAccelerated(aKey->mIsAccelerated)
>    {}
> +
> +  AllCanvasImageCacheEntry(const AllCanvasImageCacheEntry &toCopy)

const AllCanvasImageCacheEntry&

@@ +164,5 @@
>      mTotal -= aObject->SizeInBytes();
>      RemoveObject(aObject);
> +
> +    // Remove from the all canvas cache entry first since nsExpirationTracker
> +    // will delete aObject

Period.

@@ +366,5 @@
> +
> +SourceSurface*
> +CanvasImageCache::LookupCanvas(dom::Element* aImage,
> +                               dom::HTMLCanvasElement* aCanvas,
> +                               gfx::IntSize* aSize,

Drop all these namespaces; all this stuff is in scope.

It'd be nice to make |aImage| and |aCanvas| const, but don't worry about it if it's a mess. I know that kind of thing can end up propagating quite a way.

Please rename |aSize| to |aSizeOut| to indicate it's an outparam.

@@ +389,1 @@
>    *aSize = entry->mData->mSize;

Since you're unconditionally writing to |aSize|, you should probably assert that it's non-null.

::: dom/canvas/CanvasImageCache.h
@@ +53,4 @@
>                                       bool aIsAccelerated);
> +
> +private:
> +  static already_AddRefed<imgIContainer> GetImageContainer(dom::Element* aImage);

Since this is private and doesn't depend on any state from CanvasImageCache, just declare it in the .cpp file as a file-local function.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4405,5 @@
> +  if (!imgRequest) {
> +    return res;
> +  }
> +
> +  uint32_t status;

Please initialize to 0. (I know this is just copypasted, but it's easy enough.)

@@ +4505,5 @@
>        element = video;
>      }
>  
>      srcSurf =
> +        CanvasImageCache::LookupCanvas(element, mCanvasElement, &imgSize, mIsSkiaGL);

Why the four space indent? Keep it to two spaces.

@@ +4546,1 @@
>        aError.Throw(NS_ERROR_NOT_AVAILABLE);

This blank line should go.
Attachment #8744912 - Flags: review?(seth) → review+
(Assignee)

Comment 5

a year ago
Created attachment 8750367 [details] [diff] [review]
canvasCache.patch

Carrying r+, updated with review comments.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a00eb2646389
Attachment #8744912 - Attachment is obsolete: true
Attachment #8750367 - Flags: review+
(Assignee)

Comment 6

a year ago
Try looks good.
Keywords: checkin-needed
(Assignee)

Comment 7

a year ago
Created attachment 8750821 [details] [diff] [review]
canvas.patch

Updated with proper author / bug id in commit message.
Attachment #8750367 - Attachment is obsolete: true
Attachment #8750821 - Flags: review+

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1db2431203b3
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1db2431203b3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.