Closed Bug 922942 Opened 7 years ago Closed 7 years ago

Remove a bunch of callers that create Thebes contexts when we have azure content

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(15 files, 3 obsolete files)

7.33 KB, patch
roc
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
14.84 KB, patch
roc
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
10.20 KB, patch
bas.schouten
: review-
Details | Diff | Splinter Review
9.22 KB, patch
nical
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
952 bytes, patch
Details | Diff | Splinter Review
2.61 KB, patch
seth
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
2.31 KB, patch
seth
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
2.66 KB, patch
roc
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
4.18 KB, patch
roc
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
6.70 KB, patch
seth
: review+
mattwoodrow
: checkin+
Details | Diff | Splinter Review
6.04 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
5.00 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.21 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
3.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
We need to find and fix all of these so that we can guarantee all gfxContexts are backed by Azure before deleting too much code.
Attachment #812962 - Flags: review?(roc)
Attachment #812964 - Flags: review?(nical.bugzilla)
I don't think I've fixed enough of the callers to land this yet, but this is the goal :)
Attachment #812970 - Flags: review?(seth)
Attached patch Avoid thebes in imgTools (obsolete) — Splinter Review
Attachment #812971 - Flags: review?(seth)
Attached patch Avoid crashes with PointInFill (obsolete) — Splinter Review
The browser seems to start up and do basic things now without hitting any Thebes context.

Will push to try to see how many things it breaks.
Attachment #812973 - Flags: review?(bas)
Comment on attachment 812962 [details] [diff] [review]
Create Azure contexts for the reference surface

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

::: gfx/thebes/gfxPlatform.cpp
@@ +542,5 @@
> +gfxPlatform::ScreenReferenceContext()
> +{
> +  nsRefPtr<gfxContext> ctx;
> +  if (mScreenReferenceDrawTarget) {
> +    ctx = new gfxContext(mScreenReferenceDrawTarget);

Hmm. After this change, all ScreenReferenceContexts share a transform and clip, which they didn't used to.

Can we assert here that the refcount for mScreenReferenceDrawTarget is 1?
Attachment #812962 - Flags: review?(roc) → review-
> Hmm. After this change, all ScreenReferenceContexts share a transform and
> clip, which they didn't used to.
> 
> Can we assert here that the refcount for mScreenReferenceDrawTarget is 1?

Ooh, good catch.

We can definitely assert that fairly easily, RefCounted has a 'hasOneRef()' function.

Seems like a fairly general problem with the thebes wrapper, I guess it's unlikely we'd hit it in other places though.
Attachment #812973 - Flags: review?(bas) → review+
Comment on attachment 812963 [details] [diff] [review]
Don't create a Thebes context for blurring

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

I think we should let filters land next week before we land this patch, then we can make this use hardware acceleration on Windows while we're at it. If we're in a real hurry we can land this first.

::: gfx/thebes/gfxBlur.cpp
@@ +109,4 @@
>  
>      mozilla::gfx::Rect* dirtyrect = mBlur->GetDirtyRect();
>  
> +    if (aDestinationCtx->IsCairo()) {

I can't wait for this to go away ...

::: gfx/thebes/gfxBlur.h
@@ +104,5 @@
>       * The temporary alpha surface.
>       */
>      nsRefPtr<gfxImageSurface> mImageSurface;
>  
> +    nsAutoArrayPtr<unsigned char> mData;

Can we use STL instead of ns*, I believe that's the desired thing to do these days :)
Comment on attachment 812964 [details] [diff] [review]
Update BufferTextureClients using Azure

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

::: gfx/layers/ImageDataSerializer.h
@@ +40,3 @@
>  
>  protected:
> +  uint32_t GetStride() const;

This doesn't need to be a protected method
Attachment #812964 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 812970 [details] [diff] [review]
Avoid thebes in ClippedImage

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

Looks good!

I'm glad you brought this to my attention. I was preparing to land bug 764299 in the near future which included code based on this snippet from ClippedImage. I've updated the patch in bug 764299 accordingly.
Attachment #812970 - Flags: review?(seth) → review+
Comment on attachment 812971 [details] [diff] [review]
Avoid thebes in imgTools

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

::: image/src/imgTools.cpp
@@ +29,2 @@
>  using namespace mozilla::image;
> +using namespace mozilla::gfx;

Not a big fan of "using namespace"; it'd be better if you didn't add more. I'd prefer that you use individual using directives for specific identifiers, e.g. "using mozilla::RefPtr". Looks like you probably only need 3.
Attachment #812971 - Flags: review?(seth) → review+
Comment on attachment 812972 [details] [diff] [review]
Copy to gfxImageSurface using azure

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

::: gfx/thebes/gfxASurface.cpp
@@ +53,5 @@
>  #include "nsString.h"
>  #include "nsIClipboardHelper.h"
>  
>  using namespace mozilla;
> +using namespace mozilla::gfx;

Again, I suggest avoiding "using namespace".
Attachment #812972 - Flags: review?(seth) → review+
Attachment #816892 - Flags: review?(roc)
Attachment #816893 - Flags: review?(roc)
(In reply to Seth Fowler [:seth] from comment #17)
> Comment on attachment 812971 [details] [diff] [review]
> Avoid thebes in imgTools
> 
> Review of attachment 812971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgTools.cpp
> @@ +29,2 @@
> >  using namespace mozilla::image;
> > +using namespace mozilla::gfx;
> 
> Not a big fan of "using namespace"; it'd be better if you didn't add more.
> I'd prefer that you use individual using directives for specific
> identifiers, e.g. "using mozilla::RefPtr". Looks like you probably only need
> 3.

Nooooo! For years I've been telling people the exact opposite!
Here's a dev-platform thread from about a year ago where we decided "using namespace" was A-OK:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/L809txbobEU
Attachment #812960 - Flags: checkin+
Attachment #812961 - Flags: checkin+
Attachment #816892 - Flags: checkin+
Attachment #816893 - Flags: checkin+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Here's a dev-platform thread from about a year ago where we decided "using
> namespace" was A-OK:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/L809txbobEU

I find this conclusion questionable, but it's not really an important matter.
Moz2D only supports EXTEND_PAD for drawing, whereas thebes supported (and defaulted to) EXTEND_NONE.

This difference results in very slightly different results when scaling images.

This patch changes the reference image to one that was generated using EXTEND_PAD, and change the Thebes path to explicitly use EXTEND_PAD. I've checked that we pass all imgTools tests using both paths.
Attachment #812971 - Attachment is obsolete: true
Attachment #817524 - Flags: review?(seth)
Comment on attachment 817524 [details] [diff] [review]
Avoid thebes in imgTools v2

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

Per our discussion on IRC, looks good.
Attachment #817524 - Flags: review?(seth) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c610770178

Had to update a couple more of the reference images.
Assignee: nobody → matt.woodrow
Attachment #817524 - Flags: checkin+
Attachment #812964 - Flags: checkin+
Attachment #812970 - Flags: checkin+
Attachment #812972 - Flags: checkin+
Attachment #812962 - Attachment is obsolete: true
Attachment #826197 - Flags: review?(jwatt)
Updated to avoid rects with inf values for cg and d2d.
Attachment #812973 - Attachment is obsolete: true
Attachment #826199 - Flags: review?(bas)
As discussed at the summit, this behaviour isn't supported correctly for most of our backends. Cairo gets the hit testing part right, but usually draws wrong, most other backends get it wrong for both.

I don't think this behaviour is really relevant to the actual test, so changing to avoid it.
Attachment #826200 - Flags: review?(cam)
Attachment #826197 - Flags: review?(jwatt) → review+
Comment on attachment 826201 [details] [diff] [review]
Add fuzzy comparisons to a couple of tests since D2D needs it

The fact that D2D comes up with fractional results for pixel aligned circles and rects that have non-fractional x/y/w/h/cx/cy/r values is disappointing. There's no stroke involved here, so I'm not sure how that can happen. If we can't do better somehow we're going to get more than a few complaints about getBBox returning off results for sure.

One option to mitigate this issue might could be to implement a version of GetBBoxContribution that doesn't rely on PathBuilder to calculate bounds for simple shapes like <rect> and <circle>. (It'd be more efficient anyway.) That won't help <polyline>, <polygon> or <path> where the positioning is pixel aligned and with non-fractional values, but it would get the cases where content authors are most likely to notice.)
Comment on attachment 826201 [details] [diff] [review]
Add fuzzy comparisons to a couple of tests since D2D needs it

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

I agree with what Jonathan said, though.
Attachment #826201 - Flags: review?(roc) → review+
Comment on attachment 826200 [details] [diff] [review]
Change SVG test to not use overlapping strokes

This is less important than the bbox issue by a long shot, so I guess this is fine too. The same mitigation idea described in comment 36 would also work for this.
Attachment #826200 - Flags: review?(cam) → review+
Attachment #826199 - Flags: review?(bas) → review+
Depends on: 934533
(In reply to Joel Maher (:jmaher) from comment #41)
> dev.tree-management
> (https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> 8caTImH3_9o) is showing a svg regressions on most platforms for svg-asap and
> svg-opacity.  A whole set of related patches landed at the same time, this
> bug seems to reference SVG changes.
> 
> Win 7 svg-asap:
> https://datazilla.mozilla.org/
> ?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-
> Non-PGO&os=win&os_version=6.1.7601&test=tsvgx&tr_id=3438927&graph=win%206.2.
> 9200&x86=true&x86_64=false&error_bars=false&project=talos
> 
> win 7 svg opacity:
> https://datazilla.mozilla.org/
> ?start=1382979351&stop=1383584151&product=Firefox&repository=Mozilla-Inbound-
> Non-PGO&os=win&os_version=6.1.
> 7601&test=tsvgr_opacity&tr_id=3438927&graph=win%206.2.
> 9200&x86=true&x86_64=false&error_bars=false&project=talos
> 
> other platforms have a similar problem (winxp, linux)
> 
> I would like to find a fix for this since we are 10-25% regressed from
> previously.

This is really unexpected, I'll take a look at this today.
here is the main wiki for running talos locally:
https://wiki.mozilla.org/Buildbot/Talos/Running

if you want to use try server, that will get you testing on the same hardware and infrastructure, but not as fast as running locally.  

ask for help if you need any, I would be happy to pitch in.
That was fun, talos required mozinfo==0.4, and mozinstall requires mozinfo>=0.7. Fighting ensued.

Anyway, this is because we're spending a lot more time doing Path coordinate space conversions trying to match the thebes API with moz2d.

In particular we're spending about 12% of the last sub-test in tsvgx within TransformedCopyToBuilder.

This should all go away once we convert this code to use moz2d paths directly, which I believe jwatt has patches for.

Jonathan: Are those patches close to being landable?
Flags: needinfo?(jwatt)
I have an in-progress patch in bug 934183. I'd really like some Moz2D API decisions to be made by Moz2D people in bug 934182 and bug 934110 (particularly the former) to know how I'm proceeding there though. With that, I should definitely be able to land that work well before the uplift.
Flags: needinfo?(jwatt)
I have filed bug 935008 to track this.  Thanks for taking a few minutes to look into this and track down the regression.
No longer blocks: 935008
Depends on: 935008
Comment on attachment 830332 [details] [diff] [review]
Return infinite, rather than empty bounds for infinite path

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

Does this pass all tests? I changed these to return empty because it was required behaviour for our test suite iirc.
Comment on attachment 830332 [details] [diff] [review]
Return infinite, rather than empty bounds for infinite path

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

Does this pass all tests? I changed these to return empty because it was required behaviour for our test suite iirc.
Comment on attachment 812963 [details] [diff] [review]
Don't create a Thebes context for blurring

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

See previous comment.
Attachment #812963 - Flags: review?(bas) → review-
Attachment #830332 - Flags: review?(matt.woodrow)
I think we've done enough for this bug, closing and leaving bug 933019 as the tracking bug for this work.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla28
Flags: in-testsuite?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.