Stop memset()ing BGRX surfaces to 0xFF on Skia

REOPENED
Assigned to

Status

()

Core
ImageLib
REOPENED
a year ago
9 months ago

People

(Reporter: seth, Assigned: aosmond)

Tracking

({leave-open})

unspecified
mozilla54
leave-open
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(10 attachments, 20 obsolete attachments)

24.41 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
11.19 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.45 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.61 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.91 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.55 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
1.62 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.03 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
4.40 KB, patch
Details | Diff | Splinter Review
29.60 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Bug 1282496 made us memset() BGRX imgFrame surfaces to 0xFF to ensure that Skia wasn't wrong in assuming that the entire alpha channel of BGRX surfaces is 0xFF. Problem is, that's expensive and makes it difficult to downgrade surfaces from BGRA to BGRX later. (Most seriously, right now we can't downgrade JPEG images, which start as BGRA, to BGRX. This is because of the checks added in that bug.)

Let's fix this problem. We need to:

(1) Make the assert bug 1282496 was trying to avoid more precise, so it only examines the pixels which are actually sampled.

(2) Make sure we're always undersampling such that we don't include the last decoded row of the image unless we're done decoding. That is, while we're decoding, make sure that we don't draw areas of the image that haven't been written to yet. (And keep in mind that bilinear interpolation will sample a neighborhood around each pixel.)

(3) Remove the memset() and the check in imgFrame::CanOptimizeOpaqueImage() for a Skia backend. Basically, back out 1282496, though there might be some cleanup and new comments in there that are worth keeping.

(4) More of a followup, but stop starting in BGRA and downgrading to BGRX later where we can. We should just start in BGRX if at all possible. This particularly applies to the JPEG decoder.
Assignee: nobody → aosmond
(Assignee)

Comment 1

a year ago
Created attachment 8782225 [details] [diff] [review]
Part 1. Make assert more precise
(Assignee)

Comment 2

a year ago
Created attachment 8782227 [details] [diff] [review]
Part 2. Remove the memset and reenable R8G8B8X8 optimization on all backends
(Assignee)

Comment 3

a year ago
Created attachment 8782228 [details] [diff] [review]
Part 3. Start in R8B8G8X8 for JPEG decoder
(Assignee)

Comment 4

a year ago
try, with an additional change to prefer skia as a backend on Win/Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8840d8763a0e
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a year ago
Created attachment 8783042 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes.
Attachment #8782225 - Attachment is obsolete: true
Attachment #8782227 - Attachment is obsolete: true
Attachment #8782228 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Created attachment 8783044 [details] [diff] [review]
Part 2. Decoders now clear unwritten rows and start in B8G8R8X8 if possible.
(Assignee)

Updated

a year ago
Attachment #8783044 - Attachment description: Bug 1290293 - Part 2. Decoders now clear unwritten rows and start in B8G8R8X8 if possible. → Part 2. Decoders now clear unwritten rows and start in B8G8R8X8 if possible.
(Assignee)

Comment 7

a year ago
Created attachment 8783045 [details] [diff] [review]
Part 3. Remove memset to initialize surface buffers and enable B8G8R8X8 optimizations for all backends.
(Assignee)

Comment 8

a year ago
Created attachment 8783047 [details] [diff] [review]
Part 4. Add truncated image test cases.
(Assignee)

Comment 9

a year ago
try (skia forced): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2eb854edeb
try (normal): https://treeherder.mozilla.org/#/jobs?repo=try&revision=29f284bad370
(Assignee)

Comment 10

a year ago
Created attachment 8783509 [details] [diff] [review]
Part 2. Decoders now clear unwritten rows and start in B8G8R8X8 if possible. v2

Fix broken mochitests. When a BMP is embedded in an ICO it can be transparent, but our check was faulty because this can happen for any BMP, not just BPP=32.
Attachment #8783044 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8792613 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. v2

Rebase.
Attachment #8783042 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
Created attachment 8792614 [details] [diff] [review]
Part 3. Remove memset to initialize surface buffers and enable B8G8R8X8 optimizations for all backends. v2
Attachment #8783045 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
try (skia forced): https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f43b867fba
try (normal): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f3b44d42759
(Assignee)

Updated

a year ago
Depends on: 1311779
(Assignee)

Comment 14

a year ago
Created attachment 8807130 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. v3
Attachment #8792613 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
Created attachment 8807131 [details] [diff] [review]
Part 2. Decoders now clear unwritten pixels. v3
Attachment #8783509 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Created attachment 8807132 [details] [diff] [review]
Part 3. Add/update image gtests. v3
Attachment #8783047 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8807132 - Attachment description: Part 3. Add/update image gtests. → Part 3. Add/update image gtests. v3
(Assignee)

Comment 17

a year ago
Created attachment 8807133 [details] [diff] [review]
Part 4. Remove memset to initialize surface buffers. v3
Attachment #8792614 - Attachment is obsolete: true
(Assignee)

Comment 18

a year ago
Created attachment 8807153 [details] [diff] [review]
Part 2a. Make nsPNGDecoder clear on which surface format is being used. v1

Break up Part 2 into individually landable pieces to make it easier to review.
Attachment #8807131 - Attachment is obsolete: true
(Assignee)

Comment 19

a year ago
Created attachment 8807154 [details] [diff] [review]
Part 2b. Make nsBMPDecoder explicitly write transparent pixels for those skipped by delta encoding. v1
(Assignee)

Comment 20

a year ago
Created attachment 8807155 [details] [diff] [review]
Part 2c. Make nsGIFDecoder2 use B8G8R8X8 only for unpaletted frames. v1
(Assignee)

Comment 21

a year ago
Created attachment 8807156 [details] [diff] [review]
Part 2d. Make nsJPEGDecoder clear unwritten pixels if the image is truncated.
(Assignee)

Comment 22

a year ago
Created attachment 8807157 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v1
(Assignee)

Comment 23

a year ago
try (skia preferred over d2d on Windows for testing purposes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e7d68ac33663d0c633e93833cf094252e1eb72
(Assignee)

Comment 24

a year ago
Created attachment 8807230 [details] [diff] [review]
Part 4. Remove memset to initialize surface buffers for raster images. v4

Whoops, we still need to clear the buffer for rasterized SVG images. We don't need to worry about optimizations for B8G8R8A8 -> X8 however because SVGs do not call imgFrame::SetOptimizable.

try (for just linux/OSX debug reftests): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3afc8006ebf819428da4acd9cab1e39946a5499d
Attachment #8807133 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8807130 - Flags: review?(mchang)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

a year ago
Attachment #8807156 - Flags: review?(tnikkel)
Comment on attachment 8807130 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. v3

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +169,5 @@
>  
> +static IntRect
> +CalculateSurfaceBounds(const IntSize &aSize, const Rect* aBounds, const Matrix* aMatrix)
> +{
> +  IntRect maxBounds(IntPoint(0, 0), aSize);

nit: rename surfaceBounds?
Attachment #8807130 - Flags: review?(mchang) → review+
Attachment #8807153 - Flags: review?(tnikkel) → review+
Attachment #8807154 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 26

a year ago
Created attachment 8808629 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. v4 -- carries r=mchang

Fix nits.
Attachment #8807130 - Attachment is obsolete: true
Attachment #8808629 - Flags: review+
(Assignee)

Comment 27

a year ago
Created attachment 8808630 [details] [diff] [review]
Part 4. Remove memset to initialize surface buffers for raster images. v5

Based on the telemetry results from https://mzl.la/2fXmOB4, now that we start the JPEG decoder in B8G8R8X8, there are almost no images (< 1000 out of 170M opaque images) which are found to be opaque in the end, but started in B8G8R8A8. We need to be careful with this optimization because if something writes transparent pixels (e.g. a memset in the surface filter chain) that do not get overwritten with non-transparent pixels by the decoder, then we can cause a crash. Given the minimal benefit it grants, I think we can safely remove it (along with the telemetry data collection).
Attachment #8807230 - Attachment is obsolete: true
Attachment #8808630 - Flags: review?(tnikkel)
(Assignee)

Updated

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

Updated

a year ago
Attachment #8807132 - Flags: review?(tnikkel)
Attachment #8807157 - Flags: review?(tnikkel) → review?(n.nethercote)
Comment on attachment 8807157 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v1

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

Seems reasonable but I'd like tn to take a look as well.

::: image/SurfacePipe.cpp
@@ +101,5 @@
> +  const int32_t top = mWrittenRect.y + mWrittenRect.height;
> +  if (MOZ_UNLIKELY(top < height)) {
> +    const int32_t remainder = height - top;
> +    auto updateRect = IntRect(0, top, width, remainder);
> +    MOZ_ASSERT(mImageDataLength >= (uint32_t)(height * stride));

Just write `uint32_t(height * stride)`.
Attachment #8807157 - Flags: review?(tnikkel)
Attachment #8807157 - Flags: review?(n.nethercote)
Attachment #8807157 - Flags: review+
(Assignee)

Comment 29

a year ago
Created attachment 8813175 [details] [diff] [review]
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. v4.1 -- carries r=mchang

Rebase.
Attachment #8808629 - Attachment is obsolete: true
Attachment #8813175 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: leave-open

Comment 30

10 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee2e7e5a59c
Part 1. Make assert ensuring RBGX surfaces set alpha to 0xFF only check sampled bytes. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/5410a208a5cf
Part 2a. Make nsPNGDecoder clear on which surface format is being used. r=tnikkel

Comment 31

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fee2e7e5a59c
https://hg.mozilla.org/mozilla-central/rev/5410a208a5cf
Attachment #8807155 - Flags: review?(tnikkel) → review+
Comment on attachment 8807156 [details] [diff] [review]
Part 2d. Make nsJPEGDecoder clear unwritten pixels if the image is truncated.

If the image has an error we don't call FinishInternal, we call FinishWithErrorInternal. Shouldn't we do the same clear in the error case too?
Attachment #8807156 - Flags: review?(tnikkel)
Attachment #8808630 - Flags: review?(tnikkel) → review+
Comment on attachment 8807157 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v1

Same comment about FinishInternal vs FinishWithErrorInternal.
Attachment #8807157 - Flags: review?(tnikkel)
Attachment #8807132 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 34

9 months ago
(In reply to Timothy Nikkel (:tnikkel) from comment #32)
> Comment on attachment 8807156 [details] [diff] [review]
> Part 2d. Make nsJPEGDecoder clear unwritten pixels if the image is truncated.
> 
> If the image has an error we don't call FinishInternal, we call
> FinishWithErrorInternal. Shouldn't we do the same clear in the error case
> too?

The only way Decoder::HasError returns true is if Decoder::PostError was called, as that is where mError is set. In PostError we also abandon the frame in progress, if any -- so when FinishWithErrorInternal is called, mInFrame should always be false. I can add an assert for this.
(Assignee)

Comment 35

9 months ago
Created attachment 8836011 [details] [diff] [review]
Part 2f. Add assert to ensure Decoder::FinishWithErrorInternal is never called mid-frame, v1
Attachment #8836011 - Flags: review?(tnikkel)
(Assignee)

Comment 36

9 months ago
Created attachment 8836014 [details] [diff] [review]
Part 4. Remove memset to initialize surface buffers for raster images. v6 [carries r=tnikkel]

Rebase as opaque optimization already got removed in bug 1332005.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=910ff656aa6eeee99acadd800de449e31b1a0813
Attachment #8808630 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8836014 - Attachment description: Part 4. Remove memset to initialize surface buffers for raster images. v6 → Part 4. Remove memset to initialize surface buffers for raster images. v6 [carries r=tnikkel]
Attachment #8836014 - Flags: review+
(Assignee)

Comment 37

9 months ago
Created attachment 8836195 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v2

Hook into the underlying buffer state and avoid memset'ing to 0 if we can because it was already initialized.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98aa6811a85a34fe04cb7cb807dc337324c445e8
Attachment #8807157 - Attachment is obsolete: true
(Assignee)

Comment 38

9 months ago
Created attachment 8836207 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v3

Simplify the clear value logic, Maybe is expensive in terms of the assembly generated for the clear operations which are potentially done a lot (every row, plus a few extra times) and getting the clear value should ideally just become a pointer offset and copying into a register / the stack (which it does now with Linux and gcc).
Attachment #8836195 - Attachment is obsolete: true
Attachment #8836207 - Flags: review?(tnikkel)
(Assignee)

Comment 39

9 months ago
Comment on attachment 8807156 [details] [diff] [review]
Part 2d. Make nsJPEGDecoder clear unwritten pixels if the image is truncated.

See comment 34.
Attachment #8807156 - Flags: review?(tnikkel)
Attachment #8836011 - Flags: review?(tnikkel) → review+

Comment 40

9 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe86f3a5af2
Part 2c. Make nsGIFDecoder2 use B8G8R8X8 only for unpaletted frames. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e64b3ea513a
Part 2f. Assert there is no frame on the finish decoding error path. r=tnikkel
Attachment #8807156 - Flags: review?(tnikkel) → review+
Attachment #8836207 - Flags: review?(tnikkel) → review+

Comment 41

9 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7368aa665fae
Part 2b. Make nsBMPDecoder explicitly write transparent pixels for those skipped by delta encoding. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d87488b69c18
Part 2d. Make nsJPEGDecoder clear unwritten pixels if the image is truncated. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8401d12fe936
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/652c909b75ad
Part 4. Remove surface buffer initialization for raster images.
(Assignee)

Updated

9 months ago
Keywords: leave-open

Comment 42

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8fe86f3a5af2
https://hg.mozilla.org/mozilla-central/rev/5e64b3ea513a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
The last four parts need to merge before this can be officially called fixed. (leave-open got removed when the final four parts were pushed to inbound, but parts pushed before that hadn't yet been merged to central)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6098f45a8745

mochitests (intermittent, but plenty frequent, maybe 1 in 5): https://treeherder.mozilla.org/logviewer.html#?job_id=77044403&repo=mozilla-inbound

e10s web-platform-tests (intermittent, but plenty frequent, maybe 1 in 6): https://treeherder.mozilla.org/logviewer.html#?job_id=77044215&repo=mozilla-inbound

gtests (maybe permanent, they're being awkward about getting retriggered so I don't really know): opt https://treeherder.mozilla.org/logviewer.html#?job_id=77042911&repo=mozilla-inbound and debug https://treeherder.mozilla.org/logviewer.html#?job_id=77039406&repo=mozilla-inbound

Comment 45

9 months ago
Part 3 has the wrong bug number in the commit message by the way (hence not being listed in comment #41)
(Assignee)

Updated

9 months ago
Keywords: leave-open
(Assignee)

Comment 46

9 months ago
Created attachment 8837765 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v4 [carries r=tnikkel]

Fix the broken gtests where the invalid rect did not get updated for truncated images.
Attachment #8836207 - Attachment is obsolete: true
Attachment #8837765 - Flags: review+
(Assignee)

Comment 47

9 months ago
Created attachment 8837767 [details] [diff] [review]
Part 5. Prevent partial images from being used in image layers and canvas, v1

Let's see how well this works...

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

Crashes were because the images were not fully decoded, but I only considered the painted layer draw path (the most common). We need to worry about canvas and image layers, so let's try preventing them from using partial images.

(In reply to Ian Moody [:Kwan] from comment #45)
> Part 3 has the wrong bug number in the commit message by the way (hence not
> being listed in comment #41)

Thanks, I will correct this (and the missing r=s) in the patches on the next landing attempt.
(Assignee)

Comment 48

9 months ago
Created attachment 8838359 [details] [diff] [review]
Part 2e. Make SurfacePipe users clear unwritten pixels if the image is truncated. v5 [carries r=tnikkel]

Fix gtests (again) and see if this solves other intermittent issues...

try (linux opt for potential skia BGRX crashes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4474d711705cb5f25ccea30d62242fe651b52c4
try (linux/mac debug for skia BGRX assert): https://treeherder.mozilla.org/#/jobs?repo=try&revision=551a97c5bbb159a107ec627859bc8c33bc99972f
Attachment #8837765 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.