606218 - WebGL: Y axis is upside down

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Rouget [:paul]]

WebGL on Fennec/Android is upside down.
Example:
http://learningwebgl.com/lessons/lesson14/index.html

The object is upside down on Fennec.

Comment 1

[Mathias]

Problem can be reproduced on Fennec/Mac.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Vladimir, what is the GL provider on Fennec? EGL? So can I assume that all what's needed is to flip the display in EGL?

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Set y-flip always for remote canvas

Comment 4

[Oleg Romashin (:romaxa)]

Comment on attachment 490685 [details] [diff] [review]
Set y-flip always for remote canvas

Comment from cjones
(In reply to comment #3)
> Comment on attachment 490656 [details] [diff] [review]
> Add y-flip for remote sw canvas
> 
> Don't we only want to y-flip for webgl canvases?  Remember that this code is
> used for both 2d and 3d.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Should be better y-flip handling

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

WebGL still isn't blocking fennec 2.0.  I don't want to take this patch until we get things stable on all sides (desktop WebGL, Fennec GL layers, Fennec remote layers, etc.).

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 490725 [details] [diff] [review]
Should be better y-flip handling

Not the right patch; the data should already be flipped correctly before passed over the wire.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: always pass right-side-up surface across the wire

The code was blindly using mSurface before, when rendering needs to take into account flip here.  We can just call Paint(), which will do the right thing.

We also skip calling Paint() needlessly when we're shadowed.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 504075 [details] [diff] [review]
always pass right-side-up surface across the wire

The DrawSurface->Paint fix looks OK, but we can't skip Paint(aContext...) when HasShadow() in general because the target might be for a drawWindow().  I wouldn't bother with that unless you have evidence that painting to the dummy context is going to incur the kind of overhead that would show up in profiles.  Note we do the same thing for thebeslayers, and that would be more important to optimize.  I'd like to fix that silliness in general but evidence hasn't pointed to it being an issue yet (I thought I'd filed a bug too but I can't find it).

Nit: the "call ... again" comment is somewhat misleading, in that the "again" is a non sequitur.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v2

ugh, didn't realize that drawwindow also used it.  ok, just the main fix.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 504080 [details] [diff] [review]
v2

Would still s/again// in the comment, but suits me.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/809aded51aad

.. but when I looked the code over, I think this might end up double-applying opacity, because opacity is a layer property that gets forwarded, right?  So we might have to set opacity to 1.0 before calling the second Paint.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hm yeah, my bad for not catching.  We'll DrawSurface() using OPERATOR_SOURCE with the layer opacity, which if I read cairo docs correctly says it will apply the opacity.  Could probably refactor Paint() to another method that takes an opacity param, without too much pain.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I'll post a followup here shortly.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix opacity issue with earlier patch

Here's a fix for the opacity issue.  I did the refactoring as suggested.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 506187 [details] [diff] [review]
fix opacity issue with earlier patch

># HG changeset patch
># Parent ff8f838c15e835db62595ce216fe6bc4bdb4bbe5
>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -820,6 +820,11 @@ public:
>                      LayerManager::DrawThebesLayerCallback aCallback,
>                      void* aCallbackData);
> 
>+  virtual void PaintWithOpacity(gfxContext* aContext,
>+                                LayerManager::DrawThebesLayerCallback aCallback,
>+                                void* aCallbackData,

Don't need the thebes-callback params.

r=me with that, otherwise looks good.  Yay, comments.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/44ab59b32b35

Comment 20

[Anna (Waverley)]

The bug is fixed on trunk:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a1) Gecko/20110608 Firefox/7.0a1 Fennec/7.0a1 

But on Aurora build, I cannot see the object spinning. I just see the black square. Will it be also implemented in Aurora?

Should I mark it as VERIFIED FIXED, if on Aurora is not working?

Comment 21

[Oleg Romashin (:romaxa)]

Do we have decode_on_draw fix applied on Aurora? see bug https://bugzilla.mozilla.org/show_bug.cgi?id=647802

Comment 22

[Oleg Romashin (:romaxa)]

set "image.mem.decodeondraw" -> false on android and check, if that works, then we should land also 647802