Open Bug 1290293 Opened 3 years ago Updated 5 months ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

REOPENED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: seth, Assigned: aosmond, NeedInfo)

References

Details

(Keywords: leave-open)

Attachments

(10 files, 20 obsolete files)

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
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
Attached patch Part 1. Make assert more precise (obsolete) — Splinter Review
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
Attachment #8782225 - Attachment is obsolete: true
Attachment #8782227 - Attachment is obsolete: true
Attachment #8782228 - Attachment is obsolete: true
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.
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
Depends on: 1311779
Attachment #8783509 - Attachment is obsolete: true
Attachment #8783047 - Attachment is obsolete: true
Attachment #8807132 - Attachment description: Part 3. Add/update image gtests. → Part 3. Add/update image gtests. v3
Attachment #8792614 - Attachment is obsolete: true
Break up Part 2 into individually landable pieces to make it easier to review.
Attachment #8807131 - Attachment is obsolete: true
try (skia preferred over d2d on Windows for testing purposes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e7d68ac33663d0c633e93833cf094252e1eb72
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
Attachment #8807130 - Flags: review?(mchang)
Attachment #8807153 - Flags: review?(tnikkel)
Attachment #8807154 - Flags: review?(tnikkel)
Attachment #8807155 - Flags: review?(tnikkel)
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+
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)
Attachment #8807157 - Flags: review?(tnikkel)
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+
Keywords: leave-open
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
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+
(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.
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+
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
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)
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+
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+
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/8fe86f3a5af2
https://hg.mozilla.org/mozilla-central/rev/5e64b3ea513a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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 → ---
Part 3 has the wrong bug number in the commit message by the way (hence not being listed in comment #41)
Keywords: leave-open
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+
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.
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
The leave-open keyword is there and there is no activity for 6 months.
:aosmond, maybe it's time to close this bug?
Flags: needinfo?(aosmond)

The leave-open keyword is there and there is no activity for 6 months.
:aosmond, maybe it's time to close this bug?

Flags: needinfo?(aosmond)
You need to log in before you can comment on or make changes to this bug.