canvas doesn't render in metrofx nightly

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimm, Assigned: nrc)

Tracking

Trunk
mozilla25
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
This seems to run, but doesn't display anything on the canvas surface. Probably omtc related.

It would be good to have this working so we can run the tscanvasmark talos test. Marking this as blocking bug 773817 to track however we don't have to wait on this to get a base set of tests going in talos.
(Reporter)

Updated

5 years ago
Blocks: 773817
(Reporter)

Comment 1

5 years ago
Looking at various canvas demos on the web, nothing seems to render. Switching product/component.
Component: Browser → Canvas: 2D
Product: Firefox for Metro → Core
(Reporter)

Updated

5 years ago
Summary: canvasmark doesn't render in metrofx nightly → canvas doesn't render in metrofx nightly
(Assignee)

Comment 2

5 years ago
I'm working on d3d11 OMTC at the moment. I'll look at canvas tomorrow.
Blocks: 756606
(Assignee)

Updated

5 years ago
Blocks: 895116
(Assignee)

Comment 3

5 years ago
The first patch in bug 895116 breaks all 2d canvas rendering for Windows OMTC (d3d9 and 11).
Assignee: nobody → ncameron
Component: Canvas: 2D → Graphics: Layers
(Reporter)

Comment 4

5 years ago
(In reply to Nick Cameron [:nrc] from comment #3)
> The first patch in bug 895116 breaks all 2d canvas rendering for Windows
> OMTC (d3d9 and 11).

We have a simple 2d canvas ripples demo in our tests that works, fwiw - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochiperf/res/ripples.html?force=1

Updated

5 years ago
No longer blocks: 859003
(Assignee)

Comment 5

5 years ago
Created attachment 781431 [details] [diff] [review]
use a temporary surface rather than forcing an image surface for canvas

This fixes the problem, but I haven't been able to test the basic compositor to ensure that canvases still work. dzbarsky has agreed to test it for me - thanks!
Attachment #781431 - Flags: feedback?(dzbarsky)
Comment on attachment 781431 [details] [diff] [review]
use a temporary surface rather than forcing an image surface for canvas

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

Ship it!
Attachment #781431 - Flags: feedback?(dzbarsky) → feedback+
(Assignee)

Comment 7

5 years ago
Comment on attachment 781431 [details] [diff] [review]
use a temporary surface rather than forcing an image surface for canvas

thanks for testing dzbarsky!
Attachment #781431 - Flags: review?(matt.woodrow)
Comment on attachment 781431 [details] [diff] [review]
use a temporary surface rather than forcing an image surface for canvas

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

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +89,5 @@
>                              ? gfxASurface::ImageFormatRGB24
>                              : gfxASurface::ImageFormatARGB32;
>  
> +    bool needsTempSurface = !aDestSurface ||
> +                            aDestSurface->GetType() != gfxASurface::SurfaceTypeImage;

Call GetAsImageSurface on aDestSurface and check if that returns nullptr, rather than checking the type of aDestSurface.
Attachment #781431 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/6846b610be41
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 11

5 years ago
This was backed out for suspicion of causing increased frequency of bug 895873:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7656ce2cd329
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

5 years ago
I also think it doesn't work, or at least only works for main thread compositing and, for reasons I don't understand, for the basic compositor.
(Assignee)

Comment 13

5 years ago
Comment on attachment 781431 [details] [diff] [review]
use a temporary surface rather than forcing an image surface for canvas

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

I which I drive by r- my own patch...

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +158,1 @@
>        mSurface = resultSurf;

This looks like it will only work in the MTC case where we use mSurface for painting. For OMTC, we should update aDestSurface from resultSurf. I don't know why this works with the basic compositor.
I was going to wait to mark this until it was confirmed to be the cause. Unfortunately, the orange persists :(

Anyway, sounds like you have other reasons for wanting it out at this point, so I won't re-land it in the mean time.
Target Milestone: mozilla25 → ---
(Assignee)

Comment 15

5 years ago
Created attachment 783577 [details] [diff] [review]
another go at using a temporary surface
Attachment #781431 - Attachment is obsolete: true
Attachment #783577 - Flags: review?(matt.woodrow)
(In reply to Nick Cameron [:nrc] from comment #11)
> This was backed out for suspicion of causing increased frequency of bug
> 895873:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7656ce2cd329

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/7656ce2cd329
Attachment #783577 - Flags: review?(matt.woodrow) → review+

Updated

5 years ago
Blocks: 883959
https://hg.mozilla.org/mozilla-central/rev/88919d816c32
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Reporter)

Comment 20

5 years ago
CanvasMark is running great, thanks!
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.