Closed Bug 1157984 Opened 5 years ago Closed 5 years ago

Incorrect CSS 3D display

Categories

(Core :: Web Painting, defect)

37 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: alex, Assigned: kip)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webvr][vrm2])

Attachments

(9 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

1. View sample cubic CSS 3D panorama at http://walk-inside.com/sample1/?format=0
2. Rotate panorama by moving mouse around a green circle (pano location) on a floor plan on the left side on of the window.


Actual results:

At certain rotation angles one or more of cubic tiles disappear, see attached screenshot.


Expected results:

Safari, Chrome, IE10+ display that page without such issues.
Component: Untriaged → Layout: View Rendering
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
This has recently regressed, within the last few days.
(In reply to :kip (Kearwood Gilbert) from comment #2)
> This has recently regressed, within the last few days.
Previously was working in Nightly, recent Nightly build failed.  (Tested with the OpenGL compositor)
Must have been fixed and then broken again because the original bug was reported on 37. I see it on nightly and 37.
It regressed since 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b53c2753ce9a&tochange=7fc96293ada8

Before that, the black area of bad rendering was smaller.
(In reply to Timothy Nikkel (:tn) from comment #4)
> Must have been fixed and then broken again because the original bug was
> reported on 37. I see it on nightly and 37.
Yes, it was previously corrected in Nightly with Bug 1035611
Blocks: 1158562
No longer blocks: 1158562
Blocks: 1072898
Assignee: nobody → kgilbert
This is blocking WebVR work, so I'll take it.
Whiteboard: [webvr]
Blocks: 1186575
Whiteboard: [webvr] → [webvr][vrm2]
Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Attachment #8644716 - Flags: review?(vladimir)
Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Attachment #8644717 - Flags: review?(vladimir)
Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Attachment #8644718 - Flags: review?(vladimir)
Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Attachment #8644719 - Flags: review?(vladimir)
I have pushed to try for all platforms, as these changes impact platform specific code:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b79c7d7b2c0

Review has been posted; however, a test is expected to be added before landing in m-c.
(In reply to :kip (Kearwood Gilbert) from comment #8)
> Created attachment 8644716 [details]
> MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support
> both float and double precision
> 
> Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and
> double precision
> - Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
>   and Rect to create double precision versions.

So, I hate "DblPoint" "DblRect" etc.  "PointD" and "RectD" seem much better and cleaner.  I realize it's a bit bikeshedding, but it's also a pretty core naming decision.  Also not a huge fan of using "F" as the float type template parameter.. actually calling it "FloatType" or something would be cleaner when it's used.

Also note that you set default of "typename F = float", whereas before it was actually "Float" (gfx::Float) which is defined in Types.h.  You should keep it as Float, and possibly ad gfx::Double as well.

You'll want to get r+ on this from probably both bas and jrmuizel, given the core Moz2D change.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> (In reply to :kip (Kearwood Gilbert) from comment #8)
> > Created attachment 8644716 [details]
> > MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support
> > both float and double precision
> > 
> > Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and
> > double precision
> > - Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
> >   and Rect to create double precision versions.
> 
> So, I hate "DblPoint" "DblRect" etc.  "PointD" and "RectD" seem much better
> and cleaner.  I realize it's a bit bikeshedding, but it's also a pretty core
> naming decision.  Also not a huge fan of using "F" as the float type
> template parameter.. actually calling it "FloatType" or something would be
> cleaner when it's used.
I was struggling to come up with a name for the double precision equivalents and am not particularly set on the "Dbl" prefix.  I like PointD equally as much; however, would it get confusing for Point3D and Point4D..  Would they become Point3DD and Point4DD?

> 
> Also note that you set default of "typename F = float", whereas before it
> was actually "Float" (gfx::Float) which is defined in Types.h.  You should
> keep it as Float, and possibly ad gfx::Double as well.
Okay, I'll update the patches to reflect this.

> 
> You'll want to get r+ on this from probably both bas and jrmuizel, given the
> core Moz2D change.
Okay, sounds like a good idea.
(In reply to :kip (Kearwood Gilbert) from comment #14)
> I was struggling to come up with a name for the double precision equivalents
> and am not particularly set on the "Dbl" prefix.  I like PointD equally as
> much; however, would it get confusing for Point3D and Point4D..  Would they
> become Point3DD and Point4DD?

Hmm. Ew. Right.  Maybe PointD3D, PointD4D?  (If only we had called them Point3 and Point4, or even Vec3 and Vec4! :)  DblPoint3D looks worse than any of those alternatives, tbh.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #15)
> (In reply to :kip (Kearwood Gilbert) from comment #14)
> > I was struggling to come up with a name for the double precision equivalents
> > and am not particularly set on the "Dbl" prefix.  I like PointD equally as
> > much; however, would it get confusing for Point3D and Point4D..  Would they
> > become Point3DD and Point4DD?
> 
> Hmm. Ew. Right.  Maybe PointD3D, PointD4D?  (If only we had called them
> Point3 and Point4, or even Vec3 and Vec4! :)  DblPoint3D looks worse than
> any of those alternatives, tbh.
As long as nobody else (bas and jmuizelaar?) has any objection, I can make it:

Double Point -> PointD
Double Point3D -> PointD3D
Double Point4D -> PointD4D
Double Rect -> RectD
Double Size -> SizeD
Double Margin -> MarginD
Double Coord -> CoordD

I like vec3/vec3d and vec4/vec4d better, but that would be a bigger patch ;-)
(In reply to :kip (Kearwood Gilbert) from comment #12)
> I have pushed to try for all platforms, as these changes impact platform
> specific code:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b79c7d7b2c0
> 
> Review has been posted; however, a test is expected to be added before
> landing in m-c.
Try run passed...
Blocks: 1187199
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Comment on attachment 8644719 [details]
MozReview Request: Bug 1157984 - Part 4: Remove gfxRect::TransformBounds

Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Patches have been updated based on feedback in Comment 13 - Comment 16.
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

https://reviewboard.mozilla.org/r/15303/#review14427

r+ from me, but you need bas/jeff to confirm they like the naming and classes
Attachment #8644716 - Flags: review?(vladimir)
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

https://reviewboard.mozilla.org/r/15303/#review14429
Attachment #8644716 - Flags: review+
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

https://reviewboard.mozilla.org/r/15305/#review14431

::: gfx/2d/Matrix.cpp:594
(Diff revision 2)
> +  F max_y = -std::numeric_limits<F>::max();

I think we require C++11 now, so could use std::numeric_limits<F>::lowest() here, instead of -max() ?  Probably doesn't matter.
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

https://reviewboard.mozilla.org/r/15305/#review14433
Attachment #8644717 - Flags: review+
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

https://reviewboard.mozilla.org/r/15307/#review14435

::: gfx/layers/LayerTreeInvalidation.cpp:48
(Diff revision 2)
> +                                           -std::numeric_limits<int32_t>::max() * 0.5f,

Same as before with lowest(), but please add a comment explaining the magic values (specificlly, the * 0.5f?)

::: gfx/layers/composite/LayerManagerComposite.cpp:953
(Diff revision 2)
> +                        -std::numeric_limits<int32_t>::max() * 0.5,

Same as above -- maybe lowest(), and document why x/y = lowest()/2.  Is there any reason to explicitly clip to these bounds?

::: gfx/src/nsRegion.cpp:618
(Diff revision 2)
> +      -std::numeric_limits<int32_t>::max() * 0.5,

... actually, if this is going to keep appearing, we should really give this Rect a name, e.g. gfx::RectD::kMaxIntRect or something.
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Attachment #8644718 - Flags: review?(vladimir)
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Comment on attachment 8644719 [details]
MozReview Request: Bug 1157984 - Part 4: Remove gfxRect::TransformBounds

Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Hello Bas!

I believe you may want to have some input on this patch.  Would you have a few cycles to review it?
Attachment #8644716 - Flags: review+ → review?(bas)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #27)
> Comment on attachment 8644718 [details]
> MozReview Request: Bug 1157984 - Part 3: Correct bounding box
> transformations to support projections and correct clipping when
> transforming behind the camera
> 
> https://reviewboard.mozilla.org/r/15307/#review14435
> 
> ::: gfx/layers/LayerTreeInvalidation.cpp:48
> (Diff revision 2)
> > +                                           -std::numeric_limits<int32_t>::max() * 0.5f,
> 
> Same as before with lowest(), but please add a comment explaining the magic
> values (specificlly, the * 0.5f?)
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp:953
> (Diff revision 2)
> > +                        -std::numeric_limits<int32_t>::max() * 0.5,
> 
> Same as above -- maybe lowest(), and document why x/y = lowest()/2.  Is
> there any reason to explicitly clip to these bounds?
> 
> ::: gfx/src/nsRegion.cpp:618
> (Diff revision 2)
> > +      -std::numeric_limits<int32_t>::max() * 0.5,
> 
> ... actually, if this is going to keep appearing, we should really give this
> Rect a name, e.g. gfx::RectD::kMaxIntRect or something.

I've updated the patches with this feedback.

There is now a static method, gfx::Rect<...>::MaxIntRect() and some comments describing why the 0.5 value is used.

Our OSX build chain does not appear to support std::numeric_limits<int32_t>::lowest(), so I have left it as the negative of the max() for now.
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

https://reviewboard.mozilla.org/r/15303/#review14543

Ship It!

::: gfx/2d/Point.h:67
(Diff revision 3)
> -template<class units>
> +template<class units, typename F = Float>

nit: For consistency use class here instead of typename like you're going everywhere else.
Attachment #8644716 - Flags: review?(bas) → review+
Flags: needinfo?(bas)
The Part 1 patch has been updated with Bas's feedback in Comment 35
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Attachment #8644716 - Flags: review?(vladimir)
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Comment on attachment 8644719 [details]
MozReview Request: Bug 1157984 - Part 4: Remove gfxRect::TransformBounds

Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Hello Jeff,

Would you like to have some input on the naming of these classes or give it a review?
Attachment #8644716 - Flags: review?(vladimir) → review?(jmuizelaar)
Flags: needinfo?(jmuizelaar)
Attachment #8644716 - Flags: review?(jmuizelaar) → review+
Attachment #8644716 - Flags: review+ → review-
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

https://reviewboard.mozilla.org/r/15303/#review14779

Ship It!

::: gfx/2d/Point.h:136
(Diff revision 4)
>  

I think 'D' is an unclear suffix to use. I would prefer just using 'Double' even if it is a bit more awkward. The 'D' prefix is especially confusing in something like 'PointD3D' where it evokes Direct3D a lot more than double.

::: gfx/2d/Rect.h:56
(Diff revision 4)
>  

I don't think 'D' is a sufficient suffix. It is especially ambiguous in the case of MatrixD3D. I think it's best just to use 'Double' even if it is a bit more awkward.
Attachment #8644716 - Flags: review+
Appologies for my stumbling with reviewboard. This was supposed to be a r- with the comments only once. Oh well..
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Attachment #8644716 - Flags: review?(vladimir)
Attachment #8644716 - Flags: review?(jmuizelaar)
Attachment #8644716 - Flags: review-
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Comment on attachment 8644719 [details]
MozReview Request: Bug 1157984 - Part 4: Remove gfxRect::TransformBounds

Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Clearing Vlad's review request on the first patch, as he has already r+'ed.
Attachment #8644716 - Flags: review?(vladimir)
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

https://reviewboard.mozilla.org/r/15303/#review14845

Ship It!
Attachment #8644716 - Flags: review?(jmuizelaar) → review+
Minimum test case, to be converted to reftest.  Shows a green square if passed or blank page if failed.
Comment on attachment 8644716 [details]
MozReview Request: Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision

Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision
- Implemented templates for Coord, Point, Point3D, Point4D, Size, Margin
  and Rect to create double precision versions.
Attachment #8644716 - Flags: review?(vladimir)
Comment on attachment 8644717 [details]
MozReview Request: Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4

Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4
- Implement Matrix4x4::TransformAndClipBounds
- Update methods in Matrix4x4 with templates, allowing for both single
  and double precision.
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera
- Update callsites of Matrix4x4::TransformBounds to use
  Matrix4x4::TransformAndClipBounds.
Comment on attachment 8644719 [details]
MozReview Request: Bug 1157984 - Part 4: Remove gfxRect::TransformBounds

Bug 1157984 - Part 4: Remove gfxRect::TransformBounds
Bug 1157984 - Part 5: Test
- Implemented a reftest to verify that the transformed element is clipped
  against the w=0 plane without disappearing
Attachment #8653062 - Flags: review?(vladimir)
(In reply to :kip (Kearwood Gilbert) from comment #56)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d85b5c45fd03

Try run is green
Comment on attachment 8644718 [details]
MozReview Request: Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera

https://reviewboard.mozilla.org/r/15307/#review16061
https://hg.mozilla.org/integration/mozilla-inbound/rev/88e2ce193067f0caa55a1f6e70e8fc2b9579cf25
Bug 1157984 - Part 1: Extend gfx::2d classes to support both float and double precision,r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/3baf8fb8d1f0e27a110c7a60c252539a90d86616
Bug 1157984 - Part 2: Implement double precision clipping functions in Matrix4x4,r=vlad

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8134f02763653040aa35520a8012a141c92a42
Bug 1157984 - Part 3: Correct bounding box transformations to support projections and correct clipping when transforming behind the camera,r=vlad

https://hg.mozilla.org/integration/mozilla-inbound/rev/821a65601cc0ce5a4961bd11e10a083f61c16f9e
Bug 1157984 - Part 4: Remove gfxRect::TransformBounds,r=vlad

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3ce51b85e3a9e79582d947bfdf3708c322328b
Bug 1157984 - Part 5: Test,r=vlad
Attached image tested on nightly build
User Agent:  (Macintosh; Intel Mac OS X 10.10.5;) Firefox Nightly/43.0a1

I would like to get a report on the status of the bug fix. Vertical black lines sometimes appear when rotating the view.
Depends on: 1205738
Depends on: 1205449
Depends on: 1205515
This issue is rather pressing for us. Is there any work around this bug? Can the problem be alleviated through CSS?

Thank you
Depends on: CVE-2016-5252
Depends on: 1376522
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.