Add downscale-on-decode support to imgIContainer::GetImageContainer

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected, firefox59 fixed)

Details

(Whiteboard: [wr-mvp] gfx-noted)

Attachments

(17 attachments, 26 obsolete attachments)

4.27 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
4.89 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
11.91 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.24 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.14 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.80 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.02 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
19.37 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.28 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.81 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.53 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.12 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.87 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
15.92 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.05 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.18 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.42 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
This happens because nsImageRenderer::BuildWebRenderDisplayItems simply passes its converted flags to RasterImage::GetImageContainer just like it did for RasterImage::Draw. However the two methods don't support the same sets of flags. While it is easy to mask it off before calling GetImageContainer, this does point out a flaw in our display list generation, in that we do not use high quality scaling. It is probable that nsImageFrame does a RequestDecodeForSize for the correct size, but GetImageContainer from the display list generation requests another decode for the image's native size. This should be corrected.
(Assignee)

Comment 1

2 years ago
STR:
1) Configure debug build with gfx.webrendest.enabled set to true.
2) Trips assert on startup (or immediately if done via about:config).
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Has STR: --- → yes
Priority: -- → P3
Whiteboard: gfx-noted
(Assignee)

Comment 2

2 years ago
Created attachment 8875405 [details] [diff] [review]
Part 1. Move RasterImage's ImageContainer state to ImageResource., v1
(Assignee)

Comment 3

2 years ago
Created attachment 8875406 [details] [diff] [review]
Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer implementations to ImageResource., v1
(Assignee)

Comment 4

2 years ago
Created attachment 8875407 [details] [diff] [review]
Part 3. Move RasterImage::GetCurrentImage to ImageResource., v1
(Assignee)

Comment 5

2 years ago
Created attachment 8875408 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetI mageContainerImpl more generic., v1
(Assignee)

Comment 6

2 years ago
Created attachment 8875409 [details] [diff] [review]
Part 5. Refactor ImageResource::GetCurrentImage to reduce code dup lication., v1
(Assignee)

Comment 7

2 years ago
Created attachment 8875410 [details] [diff] [review]
Part 6. Add support for multiple differently-sized image container s for downscale-on-decode., v1
(Assignee)

Comment 8

2 years ago
Created attachment 8875411 [details] [diff] [review]
Part 7. Remove size parameter from ImageResource::UpdateImageConta
iner., v1
(Assignee)

Updated

2 years ago
Attachment #8875411 - Attachment description: 0008-Bug-1368776-Part-7.-Remove-size-parameter-from-Image.patch → Part 7. Remove size parameter from ImageResource::UpdateImageConta iner., v1
(Assignee)

Comment 9

2 years ago
Created attachment 8875413 [details] [diff] [review]
Part 8. Fix ImageResource::GetImageContainerImpl assert to allow h
igh quality scaling., v1
(Assignee)

Updated

2 years ago
Attachment #8875413 - Attachment description: Part 9. Expose getting an image container at a given size., v1 → Part 8. Fix ImageResource::GetImageContainerImpl assert to allow h igh quality scaling., v1
(Assignee)

Comment 10

2 years ago
Created attachment 8875414 [details] [diff] [review]
Part 9. Expose getting an image container at a given size., v1
(Assignee)

Comment 11

2 years ago
Without anything using downscale-on-decode + image containers, this should have no impact on behaviour.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15e4a0f81feaec67f86f67cda421ffc1f8c1c79

There will be another change forthcoming to enable this.
(Assignee)

Updated

2 years ago
Blocks: 1366097
(Assignee)

Updated

2 years ago
See Also: → bug 1367987
(Assignee)

Updated

2 years ago
Summary: Assertion failure: Unsupported flag passed to GetImageContainer, at image/RasterImage.cpp:628 → Add downscale-on-decode support to imgIContainer::GetImageContainer
(Assignee)

Updated

2 years ago
Blocks: 1183378
(Assignee)

Updated

a year ago
Attachment #8875405 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875406 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875407 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875408 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875409 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875410 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875411 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875413 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875414 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8875817 - Flags: review?(tnikkel)
Comment on attachment 8875405 [details] [diff] [review]
Part 1. Move RasterImage's ImageContainer state to ImageResource., v1

IsUnlocked on Image (or ImageResource) should probably be implemented: mLockCount == 0 || mAnimationConsumers == 0 seems like the right way to do that.
Attachment #8875405 - Flags: review?(tnikkel) → review+
Comment on attachment 8875406 [details] [diff] [review]
Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer implementations to ImageResource., v1

>+already_AddRefed<ImageContainer>
>+ImageResource::GetImageContainerImpl(LayerManager* aManager,
>+                                     const IntSize& aSize,
>+                                     uint32_t aFlags)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(aManager);
>+  MOZ_ASSERT((aFlags & ~(FLAG_SYNC_DECODE |
>+                         FLAG_SYNC_DECODE_IF_FAST |
>+                         FLAG_ASYNC_NOTIFY))
>+               == FLAG_NONE,
>+             "Unsupported flag passed to GetImageContainer");
>+
>+  if (!IsImageContainerAvailable(aManager, aFlags)) {
>+    return nullptr;
>+  }

Shouldn't we be passing aSize to IsImageContainerAvailable so it can use it to decide if the size is too big?
(Assignee)

Comment 15

a year ago
(In reply to Timothy Nikkel (:tnikkel) from comment #13)
> Comment on attachment 8875405 [details] [diff] [review]
> Part 1. Move RasterImage's ImageContainer state to ImageResource., v1
> 
> IsUnlocked on Image (or ImageResource) should probably be implemented:
> mLockCount == 0 || mAnimationConsumers == 0 seems like the right way to do
> that.

I will follow your lead from bug 1377252 and use mAnimationConsumers directly for the check. Conveniently it is already part of ImageResource ;).
(Assignee)

Comment 16

a year ago
(In reply to Timothy Nikkel (:tnikkel) from comment #14)
> Comment on attachment 8875406 [details] [diff] [review]
> Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer
> implementations to ImageResource., v1
> 
> >+already_AddRefed<ImageContainer>
> >+ImageResource::GetImageContainerImpl(LayerManager* aManager,
> >+                                     const IntSize& aSize,
> >+                                     uint32_t aFlags)
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread());
> >+  MOZ_ASSERT(aManager);
> >+  MOZ_ASSERT((aFlags & ~(FLAG_SYNC_DECODE |
> >+                         FLAG_SYNC_DECODE_IF_FAST |
> >+                         FLAG_ASYNC_NOTIFY))
> >+               == FLAG_NONE,
> >+             "Unsupported flag passed to GetImageContainer");
> >+
> >+  if (!IsImageContainerAvailable(aManager, aFlags)) {
> >+    return nullptr;
> >+  }
> 
> Shouldn't we be passing aSize to IsImageContainerAvailable so it can use it
> to decide if the size is too big?

I've been thinking about this. There are two problems with the current approach. You are correct that we need an IsImageContainerAvailableAtSize variant to be used in conjunction with GetImageContainerAtSize. This is particularly important for vector images in bug 1366097, because we can actually upscale to a larger surface and violate the maximum texture size.

However for raster images, this doesn't make sense. We cannot upscale. An image container for a larger size will just contain the largest natively sized surface. We should continue to do this rather than refuse to create an image container, because some callers really need an image container (e.g. WebRender path) more than perfection and all callers need to support a differently sized surface than requested (at worst, a new requirement if you want to use GetImageContainerAtSize -- this can happen due to surface substitution in a more general case).

What does need to be fixed for raster images is the fact that we create a new image container for each larger-than-native-size, even though they will always contain the same surface. It should consolidate them and this can be done as part of the GetImageContainerAtSize refactoring.
(Assignee)

Comment 17

a year ago
(In reply to Andrew Osmond [:aosmond] from comment #16)
> done as part of the GetImageContainerAtSize refactoring.

Err IsImageContainerAvailableAtSize refactoring. This bug is all about GetImageContainerAtSize refactoring :).
(Assignee)

Comment 18

a year ago
Created attachment 8885701 [details] [diff] [review]
Part 1. Move RasterImage's ImageContainer state to ImageResource., v2 [carries r=tnikkel]

Remove ImageResource::IsUnlocked and check mAnimationConsumer == 0 directly (as is currently done in RasterImage::GetImageContainer/Draw and VectorImage::Draw).
Attachment #8875405 - Attachment is obsolete: true
Attachment #8885701 - Flags: review+
(Assignee)

Comment 19

a year ago
Created attachment 8885702 [details] [diff] [review]
Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer implementations to ImageResource., v2

Rebased. Patches which verify/change the size come later in the series.
Attachment #8875406 - Attachment is obsolete: true
Attachment #8875406 - Flags: review?(tnikkel)
Attachment #8885702 - Flags: review?(tnikkel)
(Assignee)

Comment 20

a year ago
Created attachment 8885703 [details] [diff] [review]
Part 3. Move RasterImage::GetCurrentImage to ImageResource., v2
Attachment #8875407 - Attachment is obsolete: true
Attachment #8875407 - Flags: review?(tnikkel)
Attachment #8885703 - Flags: review?(tnikkel)
(Assignee)

Comment 21

a year ago
Created attachment 8885705 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic., v2
Attachment #8875408 - Attachment is obsolete: true
Attachment #8875408 - Flags: review?(tnikkel)
Attachment #8885705 - Flags: review?(tnikkel)
(Assignee)

Comment 22

a year ago
Created attachment 8885706 [details] [diff] [review]
Part 5. Refactor ImageResource::GetCurrentImage to reduce code duplication., v2
Attachment #8875409 - Attachment is obsolete: true
Attachment #8875409 - Flags: review?(tnikkel)
Attachment #8885706 - Flags: review?(tnikkel)
(Assignee)

Comment 23

a year ago
Created attachment 8885708 [details] [diff] [review]
Part 6. Add support for multiple differently-sized image containers for downscale-on-decode., v2
Attachment #8875410 - Attachment is obsolete: true
Attachment #8875410 - Flags: review?(tnikkel)
Attachment #8885708 - Flags: review?(tnikkel)
(Assignee)

Comment 24

a year ago
Created attachment 8885709 [details] [diff] [review]
Part 7. Remove size parameter from ImageResource::UpdateImageContainer., v2
Attachment #8875411 - Attachment is obsolete: true
Attachment #8875411 - Flags: review?(tnikkel)
Attachment #8885709 - Flags: review?(tnikkel)
(Assignee)

Comment 25

a year ago
Created attachment 8885710 [details] [diff] [review]
Part 8. Fix ImageResource::GetImageContainerImpl assert to allow high quality scaling., v2
Attachment #8875413 - Attachment is obsolete: true
Attachment #8875413 - Flags: review?(tnikkel)
Attachment #8885710 - Flags: review?(tnikkel)
(Assignee)

Comment 26

a year ago
Created attachment 8885712 [details] [diff] [review]
Part 9. Expose getting an image container at a given size., v2

This part has changed a bit and been split. It now adds the imgIContainer::IsImageContainerAvailableAtSize method discussed, fleshes the stubs I missed in other parts of the code for imgIContainer::GetImageContainerAtSize (as well as the new method) and merely stubs them in RasterImage. The next part will add the real implementation of these methods in RasterImage.
Attachment #8875414 - Attachment is obsolete: true
Attachment #8875414 - Flags: review?(tnikkel)
Attachment #8885712 - Flags: review?(tnikkel)
(Assignee)

Comment 27

a year ago
Created attachment 8885713 [details] [diff] [review]
Part 10. Implement RasterImage::IsImageContainerAvailableAtSize and GetImageContainerAtSize., v1
Attachment #8885713 - Flags: review?(tnikkel)
(Assignee)

Comment 28

a year ago
Created attachment 8885714 [details] [diff] [review]
Part 11. Add ImageResource::GetImageContainerSize to determine the appropriate size for a requested container., v1

This patch fixes the problem where upscaled image requests will get their own container rather than reusing the original size.
Attachment #8885714 - Flags: review?(tnikkel)
(Assignee)

Comment 29

a year ago
Created attachment 8885715 [details] [diff] [review]
Part 12. Add gtests for RasterImage::GetImageContainerAtSize., v2

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ec3d31fa17908f58fc5f1a73ba042041db8f97
Attachment #8875817 - Attachment is obsolete: true
Attachment #8875817 - Flags: review?(tnikkel)
Attachment #8885715 - Flags: review?(tnikkel)
tn: review ping
Flags: needinfo?(tnikkel)
(Assignee)

Comment 31

a year ago
Created attachment 8898870 [details] [diff] [review]
Part 0. Move RasterImage's NotifyDrawingObservers to ImageResource., v1

This is a dependency of RasterImage::GetImageContainer which needs to be moved into ImageResource. It was recently added and only has dependencies inside ImageResource.
Attachment #8898870 - Flags: review+
(Assignee)

Comment 32

a year ago
Created attachment 8898871 [details] [diff] [review]
Part 1. Move RasterImage's ImageContainer state to ImageResource., v3 [carries r=tnikkel]

Refresh against tree.
Attachment #8885701 - Attachment is obsolete: true
Attachment #8898871 - Flags: review+
(Assignee)

Comment 33

a year ago
Created attachment 8898872 [details] [diff] [review]
Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer implementations to ImageResource., v3

Refresh against tree.
Attachment #8885702 - Attachment is obsolete: true
Attachment #8885702 - Flags: review?(tnikkel)
Attachment #8898872 - Flags: review?(tnikkel)
(Assignee)

Comment 34

a year ago
Created attachment 8898873 [details] [diff] [review]
Part 3. Move RasterImage::GetCurrentImage to ImageResource., v3

Refresh against tree.
Attachment #8885703 - Attachment is obsolete: true
Attachment #8885703 - Flags: review?(tnikkel)
Attachment #8898873 - Flags: review?(tnikkel)
(Assignee)

Comment 35

a year ago
Created attachment 8898874 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic., v3

Refresh against tree.
Attachment #8885705 - Attachment is obsolete: true
Attachment #8885705 - Flags: review?(tnikkel)
Attachment #8898874 - Flags: review?(tnikkel)
(Assignee)

Comment 36

a year ago
Created attachment 8898875 [details] [diff] [review]
Part 5. Refactor ImageResource::GetCurrentImage to reduce code duplication., v3

Refresh against tree.
Attachment #8885706 - Attachment is obsolete: true
Attachment #8885706 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8898875 - Flags: review?(tnikkel)
(Assignee)

Comment 37

a year ago
Created attachment 8898880 [details] [diff] [review]
Part 6. Add support for multiple differently-sized image containers for downscale-on-decode., v3

Refresh against tree.
Attachment #8885708 - Attachment is obsolete: true
Attachment #8885708 - Flags: review?(tnikkel)
Attachment #8898880 - Flags: review?(tnikkel)
(Assignee)

Comment 38

a year ago
Created attachment 8898883 [details] [diff] [review]
Part 7. Remove size parameter from ImageResource::UpdateImageContainer., v3
Attachment #8885709 - Attachment is obsolete: true
Attachment #8885709 - Flags: review?(tnikkel)
Attachment #8898883 - Flags: review?(tnikkel)
(Assignee)

Comment 39

a year ago
Created attachment 8898884 [details] [diff] [review]
Part 8. Fix ImageResource::GetImageContainerImpl assert to allow high quality scaling., v3
Attachment #8885710 - Attachment is obsolete: true
Attachment #8885710 - Flags: review?(tnikkel)
Attachment #8898884 - Flags: review?(tnikkel)
(Assignee)

Comment 40

a year ago
Created attachment 8898885 [details] [diff] [review]
Part 9. Expose getting an image container at a given size., v3
Attachment #8885712 - Attachment is obsolete: true
Attachment #8885712 - Flags: review?(tnikkel)
Attachment #8898885 - Flags: review?(tnikkel)
(Assignee)

Comment 41

a year ago
Created attachment 8898886 [details] [diff] [review]
Part 10. Implement RasterImage::IsImageContainerAvailableAtSize and GetImageContainerAtSize., v2
Attachment #8885713 - Attachment is obsolete: true
Attachment #8885713 - Flags: review?(tnikkel)
Attachment #8898886 - Flags: review?(tnikkel)
(Assignee)

Comment 42

a year ago
Created attachment 8898887 [details] [diff] [review]
Part 11. Add ImageResource::GetImageContainerSize to determine the appropriate size for a requested container., v2
Attachment #8885714 - Attachment is obsolete: true
Attachment #8885714 - Flags: review?(tnikkel)
Attachment #8898887 - Flags: review?(tnikkel)
(Assignee)

Comment 43

a year ago
Created attachment 8898888 [details] [diff] [review]
Part 12. Add gtests for RasterImage::GetImageContainerAtSize., v3
Attachment #8885715 - Attachment is obsolete: true
Attachment #8885715 - Flags: review?(tnikkel)
Attachment #8898888 - Flags: review?(tnikkel)
Priority: P3 → P1
Whiteboard: gfx-noted → [wr-mvp] gfx-noted
Target Milestone: --- → mozilla57
status-firefox56: --- → unaffected
status-firefox57: --- → unaffected
Flags: needinfo?(tnikkel)
Attachment #8898872 - Flags: review?(tnikkel) → review+
Attachment #8898873 - Flags: review?(tnikkel) → review+
Comment on attachment 8898874 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic., v3

This is going to make us decode when no sync decoding flag was passed where we didn't before?
Comment on attachment 8898883 [details] [diff] [review]
Part 7. Remove size parameter from ImageResource::UpdateImageContainer., v3

This is part 8 again, not part 7.
Attachment #8898883 - Flags: review?(tnikkel)
I have been asked to note that this appears to be causing downscaled images to become blobimages in webrender, which was half of why this was occurring: https://bugzilla.mozilla.org/show_bug.cgi?id=1402033
(Assignee)

Comment 48

a year ago
Created attachment 8911292 [details] [diff] [review]
Part 7. Remove size parameter from ImageResource::UpdateImageContainer., v4

The real part 7!
Attachment #8898883 - Attachment is obsolete: true
Attachment #8911292 - Flags: review?(tnikkel)
Attachment #8898875 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 49

a year ago
(In reply to Timothy Nikkel (:tnikkel) from comment #45)
> Comment on attachment 8898874 [details] [diff] [review]
> Part 4. Handle all potential DrawResult values to make
> Image::GetImageContainerImpl more generic., v3
> 
> This is going to make us decode when no sync decoding flag was passed where
> we didn't before?

At most, it will cause extra lookups when the DrawResult is inconclusive (INCOMPLETE, NOT_READY, TEMPORARY_ERROR). If there was no container already, it would do the lookup and potentially cause a decode, but that was regardless of the flags passed in.
:tnikkel review ping?
Flags: needinfo?(tnikkel)
Comment on attachment 8898874 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic., v3

>+  if (container) {
>+    switch (mLastImageContainerDrawResult) {
>+      case DrawResult::SUCCESS:
>+      case DrawResult::BAD_IMAGE:
>+      case DrawResult::BAD_ARGS:
>+      case DrawResult::WRONG_SIZE:
>+        return container.forget();

For wrong size don't we want to try to decode the proper size? Otherwise we wouldn't ever trigger a decode at the requested size?
Flags: needinfo?(tnikkel)
Comment on attachment 8898880 [details] [diff] [review]
Part 6. Add support for multiple differently-sized image containers for downscale-on-decode., v3

What happens if we animate the size of a layerized image? Your work that limits who many sizes we decode will presumably kick in and then what?
Attachment #8898880 - Flags: review?(tnikkel) → review+
Attachment #8898884 - Flags: review?(tnikkel) → review+
Attachment #8911292 - Flags: review?(tnikkel) → review+
Attachment #8898885 - Flags: review?(tnikkel) → review+
Attachment #8898886 - Flags: review?(tnikkel) → review+
Comment on attachment 8898887 [details] [diff] [review]
Part 11. Add ImageResource::GetImageContainerSize to determine the appropriate size for a requested container., v2

Can you put a comment in Image.h for GetImageContainerSize?
Attachment #8898887 - Flags: review?(tnikkel) → review+
Attachment #8898888 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 54

a year ago
Created attachment 8926522 [details] [diff] [review]
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic., v4

(In reply to Timothy Nikkel (:tnikkel) from comment #51)
> Comment on attachment 8898874 [details] [diff] [review]
> Part 4. Handle all potential DrawResult values to make
> Image::GetImageContainerImpl more generic., v3
> 
> >+  if (container) {
> >+    switch (mLastImageContainerDrawResult) {
> >+      case DrawResult::SUCCESS:
> >+      case DrawResult::BAD_IMAGE:
> >+      case DrawResult::BAD_ARGS:
> >+      case DrawResult::WRONG_SIZE:
> >+        return container.forget();
> 
> For wrong size don't we want to try to decode the proper size? Otherwise we
> wouldn't ever trigger a decode at the requested size?

WRONG_SIZE should never been returned by GetFrameInternal. I've moved it to assert.
Attachment #8898874 - Attachment is obsolete: true
Attachment #8898874 - Flags: review?(tnikkel)
Attachment #8926522 - Flags: review?(tnikkel)
(Assignee)

Comment 55

a year ago
Created attachment 8926523 [details] [diff] [review]
Part 11. Add ImageResource::GetImageContainerSize to determine the appropriate size for a requested container., v3 [carries r=tnikkel]

(In reply to Timothy Nikkel (:tnikkel) from comment #53)
> Comment on attachment 8898887 [details] [diff] [review]
> Part 11. Add ImageResource::GetImageContainerSize to determine the
> appropriate size for a requested container., v2
> 
> Can you put a comment in Image.h for GetImageContainerSize?

Done.
Attachment #8898887 - Attachment is obsolete: true
Attachment #8926523 - Flags: review+
(Assignee)

Comment 56

a year ago
Created attachment 8926525 [details] [diff] [review]
Part 13. Make ImageResource::GetFrameInternal also return the suggested size for the lookup., v1

Mostly prep for handling factor of 2, no real functional change otherwise.
Attachment #8926525 - Flags: review?(tnikkel)
(Assignee)

Comment 57

a year ago
Created attachment 8926529 [details] [diff] [review]
Part 14. Make ImageResource::GetImageContainerImpl handle differing suggested sizes., v1

(In reply to Timothy Nikkel (:tnikkel) from comment #52)
> Comment on attachment 8898880 [details] [diff] [review]
> Part 6. Add support for multiple differently-sized image containers for
> downscale-on-decode., v3
> 
> What happens if we animate the size of a layerized image? Your work that
> limits who many sizes we decode will presumably kick in and then what?

This patch happens! :)

Mostly, I try just try to make it go to the right ImageContainerEntry, with the final expected size to avoid bloating the array. Unfortunately we can't immediately purge the old containers because we might need to update the generation counter for the outstanding references. Oh well.
Attachment #8926529 - Flags: review?(tnikkel)
Attachment #8926522 - Flags: review?(tnikkel) → review+
Attachment #8926525 - Flags: review?(tnikkel) → review+
Attachment #8926529 - Flags: review?(tnikkel) → review+
Blocks: 1416022
Blocks: 1242969
Duplicate of this bug: 1416022
(Assignee)

Comment 59

a year ago
Created attachment 8928659 [details] [diff] [review]
Part 15. Cache flags passed to ImageResource::GetImageContainerImpl for consistency., v1

This is important when callers start calling GetImageContainerAtSize with FLAG_HIGH_QUALITY_SCALING. Without it, UpdateImageContainer may evict substituted images while we are waiting for the appropriate size to decode, as well as force factor-of-2 mode to *not* evict the surfaces for these containers.
(Assignee)

Comment 60

a year ago
Created attachment 8928660 [details] [diff] [review]
Part 16. Ensure we more consistently pass the suggested size from SurfaceCache::LookupBestMatch., v1

I relied upon getting the suggested size in earlier parts, but didn't realize it wouldn't always be present when I needed it to be. I also cleaned up the match type determination at the end of ImageSurfaceCache::LookupBestMatch to better reflect reality.
(Assignee)

Updated

a year ago
Attachment #8928659 - Flags: review?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8928660 - Flags: review?(tnikkel)
Attachment #8928659 - Flags: review?(tnikkel) → review+
Comment on attachment 8928660 [details] [diff] [review]
Part 16. Ensure we more consistently pass the suggested size from SurfaceCache::LookupBestMatch., v1

>+      } else if (aIdealKey.Size() != bestMatch->GetSurfaceKey().Size()) {
>+        // The best factor of 2 match is still decoding, but the best we've got.
>+        MOZ_ASSERT(suggestedSize != aIdealKey.Size());
>+        MOZ_ASSERT(mFactor2Mode);
>+        matchType = MatchType::SUBSTITUTE_BECAUSE_BEST;

This doesn't seem right. bestMatch is the closest match we have. It's not related to factor of 2 mode. So all this means is we are substituting a result, not that it is the best we are willing to decode.
Attachment #8928660 - Flags: review?(tnikkel)
(Assignee)

Comment 62

a year ago
(In reply to Timothy Nikkel (:tnikkel) from comment #61)
> Comment on attachment 8928660 [details] [diff] [review]
> Part 16. Ensure we more consistently pass the suggested size from
> SurfaceCache::LookupBestMatch., v1
> 
> >+      } else if (aIdealKey.Size() != bestMatch->GetSurfaceKey().Size()) {
> >+        // The best factor of 2 match is still decoding, but the best we've got.
> >+        MOZ_ASSERT(suggestedSize != aIdealKey.Size());
> >+        MOZ_ASSERT(mFactor2Mode);
> >+        matchType = MatchType::SUBSTITUTE_BECAUSE_BEST;
> 
> This doesn't seem right. bestMatch is the closest match we have. It's not
> related to factor of 2 mode. So all this means is we are substituting a
> result, not that it is the best we are willing to decode.

At this point we've determined that exactMatch == bestMatch. I restructured this a bit because I found the code misleading while I was debugging a SUBSTITUTE_BECAUSE_BEST return case (thought was wrong but was actually right) but maybe I didn't do a good job if it isn't clear :|.
(Assignee)

Comment 63

a year ago
Created attachment 8929236 [details] [diff] [review]
Part 16. Ensure we more consistently pass the suggested size from SurfaceCache::LookupBestMatch., v2

The changes in ImageSurfaceCache::LookupBestMatch should have been a no-op. The main thing I wanted to correct was the suggestedSize being returned in SurfaceCache::LookupBestMatch and fixing the move constructor.
Attachment #8928660 - Attachment is obsolete: true
Attachment #8929236 - Flags: review?(tnikkel)
Attachment #8929236 - Flags: review?(tnikkel) → review+

Comment 64

a year ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfe9e9f8800
Part 0. Move RasterImage's NotifyDrawingObservers to ImageResource. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef34a21c32b7
Part 1. Move RasterImage's ImageContainer state to ImageResource. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd9a647eca9
Part 2. Move RasterImage::GetImageContainer and UpdateImageContainer implementations to ImageResource. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f75e98b121
Part 3. Move RasterImage::GetCurrentImage to ImageResource. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebbec7a624a
Part 4. Handle all potential DrawResult values to make Image::GetImageContainerImpl more generic. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1b16e57d97
Part 5. Refactor ImageResource::GetCurrentImage to reduce code duplication. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c89d2b9f99
Part 6. Add support for multiple differently-sized image containers for downscale-on-decode. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd84718d8dbf
Part 7. Remove size parameter from ImageResource::UpdateImageContainer. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9402cc966061
Part 8. Fix ImageResource::GetImageContainerImpl assert to allow high quality scaling. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd5200c5f76
Part 9. Expose getting an image container at a given size. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/837f7f805c53
Part 10. Implement RasterImage::IsImageContainerAvailableAtSize and GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c069e359ee49
Part 11. Add ImageResource::GetImageContainerSize to determine the appropriate size for a requested container. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/4375bb4cf0a8
Part 12. Add gtests for RasterImage::GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/400c34e4ec89
Part 13. Make ImageResource::GetFrameInternal also return the suggested size for the lookup. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3f6961ec39
Part 14. Make ImageResource::GetImageContainerImpl handle differing suggested sizes. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a29d94ccac
Part 15. Cache flags passed to ImageResource::GetImageContainerImpl for consistency. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8cfa078d86
Part 16. Ensure we more consistently pass the suggested size from SurfaceCache::LookupBestMatch. r=tnikkel

Comment 65

a year ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d05e6e230306
Part 17. Fix how we could reuse an incorrect image container instead of creating a new one. r=me
Target Milestone: mozilla57 → mozilla59

Updated

a year ago
Depends on: 1423424
You need to log in before you can comment on or make changes to this bug.