Closed Bug 1285857 Opened 3 years ago Closed 3 years ago

mask-mode: luminance failure on windows with D2D1 backend

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The is because DataSourceSurfaceD2D1 doesn't support map as MapType::WRITE. 

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?q=nsCSSRendering.cpp&redirect_type=direct#5345
Attached patch use skia drawtarget (obsolete) — Splinter Review
I use the skia drawtarget when the backend is d2d1.
Attachment #8769550 - Flags: review?(mstange)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla50
Comment on attachment 8769550 [details] [diff] [review]
use skia drawtarget

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

It would be better to use CreateDrawTargetForData instead of hardcoding backend types like this.
Attachment #8769550 - Flags: review?(mstange)
I couldn't use CreateDrawTargetForData directly since I don't have data at this moment. So I use DoesBackendSupportDataDrawtarget to check if we need to change backend.
Attachment #8771251 - Flags: review?(mstange)
Is there an automated test that fails because of this bug?  If not, it seems that there should be.
Flags: needinfo?(ethlin)
Blocks: mask-ship
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #4)
> Is there an automated test that fails because of this bug?  If not, it seems
> that there should be.

Yes, the test case should be mask-mode-b.html. But for now we don't have this problem since we have a workaround to switch the backend to SKIA from D2D1 in bug 1275478. After removing the workaround in bug 1275478, we will have this problem. So I think this bug doesn't block mask-image shipping but blocks bug 1275478.
Flags: needinfo?(ethlin)
Blocks: 1275478
No longer blocks: mask-ship
Comment on attachment 8771251 [details] [diff] [review]
check if backend support data drawtarget

Ok, this is fine, thanks.

However, I think we should unify this pattern somewhat. For example, we could have a method called gfxPlatform::CreateSimilarSoftwareDrawTarget, and then the knowledge of the software fallback backend would only exist in gfxPlatform. At the moment, we're littering BackendType::SKIA all over the code and that's not great.

Can you do the cleanup as well, in a separate bug? Thank you.
Attachment #8771251 - Flags: review?(mstange) → review+
Blocks: 1289276
(In reply to Markus Stange [:mstange] from comment #6)
> Comment on attachment 8771251 [details] [diff] [review]
> check if backend support data drawtarget
> 
> Ok, this is fine, thanks.
> 
> However, I think we should unify this pattern somewhat. For example, we
> could have a method called gfxPlatform::CreateSimilarSoftwareDrawTarget, and
> then the knowledge of the software fallback backend would only exist in
> gfxPlatform. At the moment, we're littering BackendType::SKIA all over the
> code and that's not great.
> 
> Can you do the cleanup as well, in a separate bug? Thank you.

Okay, I just opened bug 1289276 to add the function.
Attachment #8769550 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bae57fa83b7
Switch to SKIA from D2D1 when rendering mask image with luminance mode. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0bae57fa83b7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.