Closed Bug 586683 Opened 14 years ago Closed 13 years ago

Images transformed using "-moz-transform: scale" don't re-sample until the transition is complete and show a stretched copy of their original state during the transition

Categories

(Core :: Web Painting, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: me, Assigned: mattwoodrow)

References

()

Details

(Keywords: regression, Whiteboard: [hardblocker][has patch][qa!])

Attachments

(8 files, 32 obsolete files)

28.67 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
9.12 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
15.82 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
11.20 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.10 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.14 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
860 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805 Firefox/4.0b3
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805 Firefox/4.0b3

Quick demo here: http://grantheaslip.com/firefox40b3_scale_demo/

In Firefox 4.0b2, images would be re-sampled as they were scaled, but in 4.0b3 the original scale of the image will be stretched until the transition is complete, at which point it is re-sampled. This even applies when the image is being scaled down, and looks a lot worse than the old behaviour.

Reproducible: Always

Steps to Reproduce:
1. Click the image at http://grantheaslip.com/firefox40b3_scale_demo/ to scale it up or down.
Actual Results:  
In 4.0b2, the image is re-sampled during the scale transition.

In 4.0b3, the original state of the image is scaled and replaced with a re-sampled copy afterwards.

Expected Results:  
The image should be re-sampled during the scale transition.
Sounds like retained layers... and I think I've seen this filed before.
blocking2.0: --- → ?
Whiteboard: DUPEME
Blocks: 564991
Is this related to Bug 583909?
Ah, that's the bug I was thinking of, yes.
Depends on: 583909
It looks like it might be similar (and might have the same underlying cause), but this doesn't seem to have anything to do with the mouse (I can trigger the transforms using the Firebug console with the exact same result).

Sorry if it's a dupe—I did my best to search for it but didn't find anything.
I should also point out that I ran into this issue at the same time and on the same page: https://bugzilla.mozilla.org/show_bug.cgi?id=586688. I'm not sure if it's related.
Yeah, the mouse is incidental; the transform is the important part.

As for finding... it's not obvious that this is a duplicate.  We should see what happens when that other bug is fixed.
Whiteboard: DUPEME
Not a regression from retained layers per se. Actually a regression from making transforms use layers, bug 505115.
Blocks: 505115
No longer blocks: 564991
I think we could use cjones' new ThebesLayer resolution API to fix this, by automatically varying the resolution as the transform changes.

Also, it would be cool if we made a ThebesLayer that contains only a single (raster) image create an ImageLayer instead. Saves copying data.
blocking2.0: ? → final+
So, is this confirmed?  Want to know if this is real and we're still in the UNCONFIRMED state here.
@Damon: I'm the original reporter, and it's still happening in exactly the same manner in 4.0b6.
OK.  Can we get a call on what we should do in Comment 8?  Does that mean cjones owns this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #8)
> Also, it would be cool if we made a ThebesLayer that contains only a single
> (raster) image create an ImageLayer instead. Saves copying data.

That idea might harmonize with bug 598874.
Attached patch BasicLayers attempt 1 (obsolete) — Splinter Review
I'm happy to take a look at this.

This patch forces the retained content to be redrawn whenever the scaling changes by more than a factor of 2x.

Only implemented for BasicLayers so far (so you'll need to disable accelerated composition on mac/windows to try it). I'll have a go at implementing the resolution API for the other backends if this seems like a good approach.
Attachment #480477 - Flags: feedback?(jones.chris.g)
Comment on attachment 480477 [details] [diff] [review]
BasicLayers attempt 1

feedback+ for approach, but this would be r- for implementation.  ThebesLayerBuffer::BeginPaint needs to know if its layer's resolution has changed so that it can invalidate etc.  So setting a new resolution needs to proceed approximately as
 (1) Determine desired resolution paintRes
 (2) Call ThebesLayerBuffer::BeginPaint(paintRes)
 (3) ThebesLayer::mResolution <- paintRes
if that makes sense.
Attachment #480477 - Flags: feedback?(jones.chris.g) → feedback+
Also, your code needs to take the BasicLayerManager's desired resolution into account, so ThebesLayers are repainted at manager->Resolution() * scaleForTransform (where your code is computing the latter).
Attached patch BasicLayers v2 (obsolete) — Splinter Review
Sorry, missed that BeginPaint already handles invalidation if the scale has changed.

The scale inversion code feels fairly ugly, can't think of an easier way to clamp to factors of 2 though. Open to suggestions!
Attachment #480477 - Attachment is obsolete: true
Attachment #481061 - Flags: review?(jones.chris.g)
Comment on attachment 481061 [details] [diff] [review]
BasicLayers v2

>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
>+  gfxSize scale = aContext->CurrentMatrix().ScaleFactors(PR_TRUE);
>+#define MAX_SCALE_FACTOR 2

|static const kFoo| please.  I find this name misleading; how about
kScaleStride or kScaleUnit or something instead?  How did you choose
the value 2?  I don't have a better suggestion, but you should at
least document your reasoning.

>+
>+  /* Convert negative scales to positive values */

ITYM convert down-scales to up-scales, or invert the scale?

>+  PRBool invertedXScale = PR_FALSE;
>+  PRBool invertedYScale = PR_FALSE;
>+  if (scale.width < 1.0) {
>+    invertedXScale = PR_TRUE;
>+    scale.width = 1 / scale.width;
>+  }
>+  if (scale.height < 1.0) {
>+    invertedYScale = PR_TRUE;
>+    scale.height = 1 / scale.height;
>+  }

Oops, I just found out that negative scales (<0.0) are allowed.  This
code is going to choke on that case, but so will the resolution-scaled
drawing code.  Maybe get the logic right here and file a followup on
the resolution-scaled drawing code?

>+
>+  /* Clamp scales to nearest MAX_SCALE_FACTOR value */
>+  scale.width = (floor(scale.width) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
>+  scale.height = (floor(scale.height) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
>+

If the intention is round the scale to a multiple of kScaleUnit, this
code doesn't implement it.

 1 / 0.03 = 33.3...
 (floor(33.3...) / 2) * 2 = 33

Except for really tiny original xscale values, I think the effect of
this code is just going to be |floor(scale)|.

>+  /* Restore negative scales */
>+  if (invertedXScale) {
>+    scale.width = 1 / scale.width;
>+  }
>+  if (invertedYScale) {
>+    scale.height = 1 / scale.height;
>+  }
>+

You should split the unit-clamping code into a helper function and
apply it to xscale/yscale.

I feel like I'm probably missing something though.  Can you explain
this patch in a little more detail?
Attachment #481061 - Flags: review?(jones.chris.g)
> kScaleStride or kScaleUnit or something instead?  How did you choose
> the value 2?  I don't have a better suggestion, but you should at
> least document your reasoning.

First number that came into my head, I'll document it as this and we can adjust it later if necessary.


> Oops, I just found out that negative scales (<0.0) are allowed.  This
> code is going to choke on that case, but so will the resolution-scaled
> drawing code.  Maybe get the logic right here and file a followup on
> the resolution-scaled drawing code?

Will do.

> 
> >+
> >+  /* Clamp scales to nearest MAX_SCALE_FACTOR value */
> >+  scale.width = (floor(scale.width) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
> >+  scale.height = (floor(scale.height) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
> >+
> 
> If the intention is round the scale to a multiple of kScaleUnit, this
> code doesn't implement it.
> 
>  1 / 0.03 = 33.3...
>  (floor(33.3...) / 2) * 2 = 33
> 
> Except for really tiny original xscale values, I think the effect of
> this code is just going to be |floor(scale)|.

Are you sure? 33/2 should be 16 (integer division), and that would result in 32. That was the intent anyway.

> 
> >+  /* Restore negative scales */
> >+  if (invertedXScale) {
> >+    scale.width = 1 / scale.width;
> >+  }
> >+  if (invertedYScale) {
> >+    scale.height = 1 / scale.height;
> >+  }
> >+
> 
> You should split the unit-clamping code into a helper function and
> apply it to xscale/yscale.

Wouldn't that still require the two local inverted variables? Doesn't seem like a lot of gain.

> 
> I feel like I'm probably missing something though.  Can you explain
> this patch in a little more detail?

Essentially the code just determines the current scale factor as a multiple (or inverse multiple?) of 2 (ie 2,4,6,8 or 1/2, 1/4 etc). This is then passed as the resolution to BeginPaint which will invalidate the content and redraw if it has changed. Ie A scale factor of 1.5 (Converts to a scale factor of 0), changing to 2.5 (Converts to a scale factor of 2) will trigger a redraw at double resolution, but changing to 1.9 (Still scale factor of 0) wouldn't.
> > >+
> > >+  /* Clamp scales to nearest MAX_SCALE_FACTOR value */
> > >+  scale.width = (floor(scale.width) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
> > >+  scale.height = (floor(scale.height) / MAX_SCALE_FACTOR) * MAX_SCALE_FACTOR;
> > >+
> > 
> > If the intention is round the scale to a multiple of kScaleUnit, this
> > code doesn't implement it.
> > 
> >  1 / 0.03 = 33.3...
> >  (floor(33.3...) / 2) * 2 = 33
> > 
> > Except for really tiny original xscale values, I think the effect of
> > this code is just going to be |floor(scale)|.
> 
> Are you sure? 33/2 should be 16 (integer division), and that would result in
> 32. That was the intent anyway.
> 

#include <math.h>
#include <stdio.h>
int main() {
    static const int stride = 2;
    float s = 0.03;
    float is = 1 / s;
    float cs = (floor(is) / stride) * stride;
    printf("%g\n", cs);
    return 0;
}
cjones@beast:/tmp[]$ gcc -o m -lm m.c 
cjones@beast:/tmp[]$ ./m
33
> > 
> > >+  /* Restore negative scales */
> > >+  if (invertedXScale) {
> > >+    scale.width = 1 / scale.width;
> > >+  }
> > >+  if (invertedYScale) {
> > >+    scale.height = 1 / scale.height;
> > >+  }
> > >+
> > 
> > You should split the unit-clamping code into a helper function and
> > apply it to xscale/yscale.
> 
> Wouldn't that still require the two local inverted variables? Doesn't seem like
> a lot of gain.
> 

I was thinking the Paint() code would look like

  gfxSize scale = aContext->CurrentMatrix().ScaleFactors(PR_TRUE);
  float paintXRes = BasicManager()->XResolution() * ClampToStride(scale.x);
  float paintYRes = BasicManager()->YResolution() * ClampToStride(scale.y);

and a single copy of the code to compute the clamp would be moved into ClampToStride() (or whatever name).

> > 
> > I feel like I'm probably missing something though.  Can you explain
> > this patch in a little more detail?
> 
> Essentially the code just determines the current scale factor as a multiple (or
> inverse multiple?) of 2 (ie 2,4,6,8 or 1/2, 1/4 etc). This is then passed as
> the resolution to BeginPaint which will invalidate the content and redraw if it
> has changed. Ie A scale factor of 1.5 (Converts to a scale factor of 0),

Oh hmm.  I thought this was a typo but your algorithm that really would convert to 0.0.  You don't want to pass that down or the resolution-scaled drawing code will take you at your word and scale by 0.0 ;).  You want this to end up at 1.0 instead.

> changing to 2.5 (Converts to a scale factor of 2) will trigger a redraw at
> double resolution, but changing to 1.9 (Still scale factor of 0) wouldn't.

OK, this makes sense.
Actually "stride" is a bad name.  "Unit" may be better but it's not great.  I don't know a good name for this, maybe "fuzz factor".  Probably want to ask someone else ;).
> Are you sure? 33/2 should be 16 (integer division)

floor doesn't return an integer.  It returns a double.  Then the integer gets promoted, etc.
And do you maybe want floorf?
Attached patch BasicLayers v3 (obsolete) — Splinter Review
Fixed (hopefully all of the) review comments.
Attachment #481061 - Attachment is obsolete: true
Attachment #481984 - Flags: review?(jones.chris.g)
Gives nsDisplayImages that are the only display item within a thebes layer their own separate ImageLayer instead.

Not sure if this is the best approach, it feels somewhat like abusing the GetLayerState API. Possibly more interested in feedback than a review as such.

Appears to work for the linked example, will need to run it through try before landing though.

We should be able to extend the logic in ProcessDisplayItems to implement the rest of bug 598784.
Attachment #482100 - Flags: review?(roc)
Uh, bug 598874.
This gives images that are the only child of an nsDisplayClip list their own layer, right? I'm not sure if we want that.

I think this would be cleaner if we passed in a parameter to GetLayerState to indicate whether this display item is the only item in the list. Make it a flags word for extensibility and readability.

However, this isn't exactly what we want, is it? What if a display list contains two elements, an image and a video? The video gets its own layer, but the image isn't the only child so we'd still create a ThebesLayer for it. Maybe we want something more like the ColorLayer optimization. So down where we decide whether to create a ColorLayer or a ThebesLayer, we could detect if the ThebesLayer contains a single display item that can just make an ImageLayer (or maybe a generic Layer) for us. It would need be recycled just like the ColorLayers and ThebesLayers are.
(In reply to comment #26)
> This gives images that are the only child of an nsDisplayClip list their own
> layer, right? I'm not sure if we want that.

I think it should work when it's the only child of any container layer. In the example URL, the nsDisplayImage is the only child of a nsDisplayTransform, and it appears to work for that.

> 
> I think this would be cleaner if we passed in a parameter to GetLayerState to
> indicate whether this display item is the only item in the list. Make it a
> flags word for extensibility and readability.

Agreed.

> 
> However, this isn't exactly what we want, is it? What if a display list
> contains two elements, an image and a video? The video gets its own layer, but
> the image isn't the only child so we'd still create a ThebesLayer for it. Maybe
> we want something more like the ColorLayer optimization. So down where we
> decide whether to create a ColorLayer or a ThebesLayer, we could detect if the
> ThebesLayer contains a single display item that can just make an ImageLayer (or
> maybe a generic Layer) for us. It would need be recycled just like the
> ColorLayers and ThebesLayers are.

I'll have a look at the ColorLayer (no u?!) code and try work something similar out!
(In reply to comment #27)
> (In reply to comment #26)
> > This gives images that are the only child of an nsDisplayClip list their own
> > layer, right? I'm not sure if we want that.
> 
> I think it should work when it's the only child of any container layer. In the
> example URL, the nsDisplayImage is the only child of a nsDisplayTransform, and
> it appears to work for that.

Right, but it also gives images their own layer if the container layer contains multiple images but each image is wrapped in a different nsDisplayClip. I don't think we want that. Anyway this is moot given the other issue I mentioned below.

> I'll have a look at the ColorLayer (no u?!) code and try work something similar
> out!

Great!

Yes, we've succumbed to US-spelling hegemony in this project.
OS: Mac OS X → Windows 7
Comment on attachment 481984 [details] [diff] [review]
BasicLayers v3

>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
>+/* Clamp aVal to a multiple (or inverse multiple) of kScaleFactor */

//-style comment please.

>+static gfxFloat
>+ClampToScaleFactor(gfxFloat aVal)
>+{
>+  /* Arbitary scale factor limitation. We can increase this
>+   * for better scaling performance at the cost of worse
>+   * quality.
>+   */
>+  static const int kScaleFactor = 2;

I realized what this really is the other day ... another resolution ;).  |kScaleResolution| wfm, but I'll leave the name to your judgement.

Also, nit: I'd prefer //-style comments throughout this function.

>+  PRBool invert = PR_FALSE;
>+  PRBool negative = PR_FALSE;
>+
>+  /* Make all scale values positive for floor() to work correctly */
>+  if (aVal < 0.0) {
>+    negative = PR_TRUE;
>+    aVal = -aVal;
>+  }
>+
>+  /* Invert negative scale values */

I still think you mean "down-scaled" rather than "negative scale" here.

Looks OK.  I thought about this patch a little more; I think what we really want in the long run is a way to throttle the scale changes by time, rather than limiting the resolution of the values.  I think you're relying on FrameLayerBuilder freezing and unfreezing for us here, which is fine.  But (I think) there will be cases where we're forced to keep a scaled thebes layer around forever, and the problem with limiting the scaling resolution is that it's impossible to get maximum-quality renderings even if the page isn't animated.  This patch is strictly better than before and should work well for common cases, so can follow up as issues arise.
Attachment #481984 - Flags: review?(jones.chris.g) → review+
> >+  static const int kScaleFactor = 2;
> 
> I realized what this really is the other day ... another resolution ;). 
> |kScaleResolution| wfm, but I'll leave the name to your judgement.

Alright, this works for me.


> >+
> >+  /* Invert negative scale values */
> 
> I still think you mean "down-scaled" rather than "negative scale" here.

Yep, missed this comment before.

> 
> Looks OK.  I thought about this patch a little more; I think what we really
> want in the long run is a way to throttle the scale changes by time, rather
> than limiting the resolution of the values.  I think you're relying on
> FrameLayerBuilder freezing and unfreezing for us here, which is fine.  But (I
> think) there will be cases where we're forced to keep a scaled thebes layer
> around forever, and the problem with limiting the scaling resolution is that
> it's impossible to get maximum-quality renderings even if the page isn't
> animated.  This patch is strictly better than before and should work well for
> common cases, so can follow up as issues arise.

This should be a fairly rare case, most time the layers should become inactive and this pass won't have the quality issue.
Attached patch BasicLayers v4 (obsolete) — Splinter Review
Fixed review comments.
Attachment #481984 - Attachment is obsolete: true
Attachment #482801 - Flags: review+
Rewritten using the ColorLayer method.

Depends on bug 599507 to get any quality improvements on OGL.
Attachment #482100 - Attachment is obsolete: true
Attachment #482802 - Flags: review?(roc)
Attachment #482100 - Flags: review?(roc)
Depends on: 599507
+  if (aItem->GetType() == nsDisplayItem::TYPE_IMAGE && mVisibleRegion == nsIntRegion()) {

mVisibleRegion.IsEmpty()

+    if (image->GetImage()->GetType() == imgIContainer::TYPE_RASTER) {
+      mImage = image;

Better to do this check later in PopThebesLayerData. In particular we could do the check in nsImageFrame::GetContainer, allowing GetContainer to return null in which case PopThebesLayerData will bail and stop trying the optimization.

In fact, in PopThebesLayerData I think the imagelayer handling code can mostly be broken into two parts: code that can be shared with the colorlayer path, and code that can be moved into nsImageFrame/nsDisplayImage. Especially all the transform stuff can be moved out of here into nsImageFrame. Maybe we should call GetContainer first, then if it returns non-null create the imagelayer and call another nsDisplayImage method to configure the imagelayer?
Fixed review suggestions.

Added fast path for CairoImageOGL::SetData.

With this animation we render the first frame using BasicLayers (since it defaults to inactive), and then switch to active layers (and thus hit the texture uploading slowdown) on the second frame). Is there any way to detect that this will be always active and skip the first inactive pass?
Attachment #482802 - Attachment is obsolete: true
Attachment #483119 - Flags: review?(roc)
Attachment #482802 - Flags: review?(roc)
+    NS_ASSERTION(aContainer->Manager() == Manager(), "ImageContainers must have the same manager as the ImageLayer!");

80 column warning

+  if (type == gfxASurface::SurfaceTypeImage)
+  {
+    imageSurface = static_cast<gfxImageSurface*>(aData.mSurface);
+  }
+#ifdef XP_MACOSX
+  else if (type == gfxASurface::SurfaceTypeQuartzImage)
+  {
+    nsRefPtr<gfxQuartzImageSurface> quartz = static_cast<gfxQuartzImageSurface*>(aData.mSurface);
+    imageSurface = quartz->GetImageSurface();
+  }
+#endif

We should move this to a utility function that tries to get a gfxImageSurface from an arbitrary cairo surface without copying, returning null if that's not possible. You can do this for gfxQuartzSurfaces as well as gfxQuartzImageSurfaces (and gfxWindowsSurfaces too, although that doesn't matter here), although it seems we don't expose the APIs you'd need to do this for gfxQuartzSurfaces. I have a feeling we want this in other places.

+  if (imageSurface && imageSurface->Format() != gfxASurface::ImageFormatARGB32) {

You should also check the stride here. I believe fTexSubImage2D requires the stride to be 4*width, which isn't necessarily the case for an arbitrary gfxImageSurface.

The CairoImageOGL optimization should be moved to a separate patch.

It could also be improved further on X systems that support texture-to-pixmap; I think we could avoid reading back the image data. In fact, we really don't want to end up reading back image data on X. I think we need a way to ask the ImageContainer if a given surface is fast with CairoImage; then you should call that in CanOptimizeImageLayer and don't try your optimization if CairoImage is going to be slow (i.e. involve readback).

+    if (data->mIsSolidColorInVisibleRegion) {
+      // Copy transform and clip rect
+      layer->SetTransform(data->mLayer->GetTransform());

Shouldn't we be doing something with the layer's old transform and cliprect? It seems like this patch just drops them on the floor.

+  transform.Scale(float(destSize.width)/imageWidth,
+                    float(destSize.height)/imageHeight);

Fix indent

The rest looks good.
(In reply to comment #34)
> With this animation we render the first frame using BasicLayers (since it
> defaults to inactive), and then switch to active layers (and thus hit the
> texture uploading slowdown) on the second frame). Is there any way to detect
> that this will be always active and skip the first inactive pass?

It's not always active; the animation doesn't start until you click. Are you saying the first changed frame after the click is painted using an inactive layer as well? That seems wrong.
(In reply to comment #35)
> It could also be improved further on X systems that support texture-to-pixmap;
> I think we could avoid reading back the image data. In fact, we really don't
> want to end up reading back image data on X. I think we need a way to ask the
> ImageContainer if a given surface is fast with CairoImage; then you should call
> that in CanOptimizeImageLayer and don't try your optimization if CairoImage is
> going to be slow (i.e. involve readback).

Or maybe we can just ensure (and document the requirement) that for any given layer manager and gfxASurface, using a CairoImage for the gfxASurface is no slower than drawing the gfxASurface to a ThebesLayer.
(In reply to comment #29)
> Looks OK.  I thought about this patch a little more; I think what we really
> want in the long run is a way to throttle the scale changes by time, rather
> than limiting the resolution of the values.

We need to limit the amount of scaling we do during the transition even if we're also throttling by time. In particular we don't want to be showing massively scaled-up images at any time.
That patch doesn't cap scale values, it just rounds them to the nearest multiple of the scale granularity.  I would think that the CSS or layer-builder code would cap the values, but I can the difficulty in that with arbitrarily nested transforms.
The patch bounds the amount of scaling of a ThebesLayer at composition time to between 0.5 and 2, doesn't it?

Hmm, personally I would have written ClampToScaleFactor to use kScaleResolution^(round(log(aVal)/log(kScaleResolution)), rounding to the nearest power of kScaleResolution.
(In reply to comment #40)
> The patch bounds the amount of scaling of a ThebesLayer at composition time to
> between 0.5 and 2, doesn't it?
> 

No.  It rounds the scale to multiples of 2 for >=1 and 0.5 for <1.

> Hmm, personally I would have written ClampToScaleFactor to use
> kScaleResolution^(round(log(aVal)/log(kScaleResolution)), rounding to the
> nearest power of kScaleResolution.

Sure, that's another heuristic.  Your feel for this is strictly better than mine, so I'll leave this between you and Matt :).
(In reply to comment #41)
> (In reply to comment #40)
> > The patch bounds the amount of scaling of a ThebesLayer at composition time to
> > between 0.5 and 2, doesn't it?
> 
> No.  It rounds the scale to multiples of 2 for >=1 and 0.5 for <1.

That's within the bounds I mentioned :-).

> > Hmm, personally I would have written ClampToScaleFactor to use
> > kScaleResolution^(round(log(aVal)/log(kScaleResolution)), rounding to the
> > nearest power of kScaleResolution.
> 
> Sure, that's another heuristic.  Your feel for this is strictly better than
> mine, so I'll leave this between you and Matt :).

OK Matt, I do think we should do that change. At high or low scale factors we'll be redrawing more than we need to, otherwise.
(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > The patch bounds the amount of scaling of a ThebesLayer at composition time to
> > > between 0.5 and 2, doesn't it?
> > 
> > No.  It rounds the scale to multiples of 2 for >=1 and 0.5 for <1.
> 
> That's within the bounds I mentioned :-).

Scale 6.5 --> rounded to 6.0, e.g.
Depends on: 605057
(In reply to comment #35)
> +    NS_ASSERTION(aContainer->Manager() == Manager(), "ImageContainers must
> have the same manager as the ImageLayer!");
> 
> 80 column warning

Fixed.

> 
> +  if (type == gfxASurface::SurfaceTypeImage)
> +  {
> +    imageSurface = static_cast<gfxImageSurface*>(aData.mSurface);
> +  }
> +#ifdef XP_MACOSX
> +  else if (type == gfxASurface::SurfaceTypeQuartzImage)
> +  {
> +    nsRefPtr<gfxQuartzImageSurface> quartz =
> static_cast<gfxQuartzImageSurface*>(aData.mSurface);
> +    imageSurface = quartz->GetImageSurface();
> +  }
> +#endif
> 
> We should move this to a utility function that tries to get a gfxImageSurface
> from an arbitrary cairo surface without copying, returning null if that's not
> possible. You can do this for gfxQuartzSurfaces as well as
> gfxQuartzImageSurfaces (and gfxWindowsSurfaces too, although that doesn't
> matter here), although it seems we don't expose the APIs you'd need to do this
> for gfxQuartzSurfaces. I have a feeling we want this in other places.

I've filed bug 605057 for this.

> 
> +  if (imageSurface && imageSurface->Format() !=
> gfxASurface::ImageFormatARGB32) {
> 
> You should also check the stride here. I believe fTexSubImage2D requires the
> stride to be 4*width, which isn't necessarily the case for an arbitrary
> gfxImageSurface.
> 
> The CairoImageOGL optimization should be moved to a separate patch.

Done and done!

> 
> It could also be improved further on X systems that support texture-to-pixmap;
> I think we could avoid reading back the image data. In fact, we really don't
> want to end up reading back image data on X. I think we need a way to ask the
> ImageContainer if a given surface is fast with CairoImage; then you should call
> that in CanOptimizeImageLayer and don't try your optimization if CairoImage is
> going to be slow (i.e. involve readback).

ImageLayerOGL::SetData already attempts to do this!

> 
> +    if (data->mIsSolidColorInVisibleRegion) {
> +      // Copy transform and clip rect
> +      layer->SetTransform(data->mLayer->GetTransform());
> 
> Shouldn't we be doing something with the layer's old transform and cliprect? It
> seems like this patch just drops them on the floor.

I'm not sure how to do this exactly. For the given example ToReferenceFrame() returns 670,263 and the existing transform is 0,63 (63 is the height of the chrome for me). Using 670,263 renders the image in the correct place, and I'm not sure where I'd get the missing 670,200 required to use the existing transform.
Changes as above.
Attachment #483119 - Attachment is obsolete: true
Attachment #483887 - Flags: review?(roc)
Attachment #483119 - Flags: review?(roc)
Attached patch Optimize CairoImageOGL::SetData (obsolete) — Splinter Review
Attachment #483888 - Flags: review?(roc)
+  nsIntPoint dest = ToReferenceFrame().ToNearestPixels(factor);
+  transform.Translate(gfxPoint(dest.x, dest.y));
+  nsIntRect destSize = imageFrame->GetInnerArea().ToNearestPixels(factor);
+  PRInt32 imageWidth;
+  PRInt32 imageHeight;
+  mImage->GetWidth(&imageWidth);
+  mImage->GetHeight(&imageHeight);
+  transform.Scale(float(destSize.width)/imageWidth,
+                  float(destSize.height)/imageHeight);

This will cause a bug when GetInnerArea().TopLeft() is nonzero; you're just forgetting about it.

Add ToReferenceFrame() to GetInnerArea(), then build your transform based on that rect.

Otherwise looks good.
Comment on attachment 483888 [details] [diff] [review]
Optimize CairoImageOGL::SetData

+      (imageSurface->Format() != gfxASurface::ImageFormatARGB32 ||
+      imageSurface->Stride() != imageSurface->Width() * 4))

Fix indent!
Attachment #483888 - Flags: review?(roc) → review+
Attached patch BasicLayers v5 (obsolete) — Splinter Review
> > > Hmm, personally I would have written ClampToScaleFactor to use
> > > kScaleResolution^(round(log(aVal)/log(kScaleResolution)), rounding to the
> > > nearest power of kScaleResolution.
> > 
> > Sure, that's another heuristic.  Your feel for this is strictly better than
> > mine, so I'll leave this between you and Matt :).
> 
> OK Matt, I do think we should do that change. At high or low scale factors
> we'll be redrawing more than we need to, otherwise.

I think this was the heuristic I originally wanted. :)
Attachment #482801 - Attachment is obsolete: true
Attachment #483893 - Flags: review?(roc)
Comment on attachment 483893 [details] [diff] [review]
BasicLayers v5

+  // Negative scaling is just a flap and irrelevant to

ITYM "flip"
Attachment #483893 - Flags: review?(roc) → review+
Updated for bug 605057 changes
Attachment #483888 - Attachment is obsolete: true
Attachment #484567 - Flags: review+
Attached patch BasicLayers v6 (obsolete) — Splinter Review
Fixed windows compilation errors.
Attachment #483893 - Attachment is obsolete: true
Attachment #484568 - Flags: review+
Fixed review comments.

Fixed memory leak and try server errors.
Attachment #483887 - Attachment is obsolete: true
Attachment #484569 - Flags: review?(roc)
Attachment #483887 - Flags: review?(roc)
+  nsIntPoint dest = ToReferenceFrame().ToNearestPixels(factor);
+  nsIntRect destSize = imageFrame->GetInnerArea().ToNearestPixels(factor);
+  destSize += dest;

Don't do ToNearestPixels here, just divide. The ImageLayer will snap.
The transform needs to have integer values for the translation so we can shift the visible region. This will fail the assertion on line 897.
Can you rebase this on top of 602200? the snapping stuff will change
Assignee: nobody → matt.woodrow+bugzilla
Depends on: 602200
Rebased on top of 602200.
Attachment #484569 - Attachment is obsolete: true
Attachment #489069 - Flags: review?(roc)
Attachment #484569 - Flags: review?(roc)
+  /* Mark as available for coversion to image layer if this is a nsDisplayImage and

conversion

+  destSize.x /= factor; destSize.y /= factor;
+  destSize.width /= factor; destSize.height /= factor;

You need to get destSize into a gfxRect before you do this division.

HasNonIntegerTranslationAndScale doesn't make much sense to me. It doesn't seem generally useful since such a transform doesn't have useful properties, for example it doesn't transform pixel-aligned rectangles to pixel-aligned rectangles. So can we change this assertion back?
+    NS_ASSERTION(!transform.HasNonIntegerTranslationAndScale(),
> HasNonIntegerTranslationAndScale doesn't make much sense to me. It doesn't seem
> generally useful since such a transform doesn't have useful properties, for
> example it doesn't transform pixel-aligned rectangles to pixel-aligned
> rectangles. So can we change this assertion back?
> +    NS_ASSERTION(!transform.HasNonIntegerTranslationAndScale(),

Right, this was from before we adding the snapping code. Thinking about it, it probably wasn't correct then either, removed.
Attachment #489069 - Attachment is obsolete: true
Attachment #489426 - Flags: review?(roc)
Attachment #489069 - Flags: review?(roc)
Comment on attachment 489426 [details] [diff] [review]
Give Images their own layer sometimes. v7

Almost there, but...

+  aLayer->SetVisibleRegion(gfxThebesUtils::GfxRectToIntRect(destRect));

This isn't right ... layer visible regions are *before* transform, i.e. before the image is scaled, but imageFrame->GetInnerArea() is *after* the image is scaled.
Attachment #489426 - Flags: review?(roc) → review-
Fixed visible region in ConfigureLayer
Attachment #489426 - Attachment is obsolete: true
Attachment #489940 - Flags: review?(roc)
Not really part of this bug (I can file a separate bug if needed), but I needed part of it for the following patch and it seemed worthwhile.
Attachment #491750 - Flags: review?(roc)
Attachment #491749 - Flags: review?(roc)
Comment on attachment 491750 [details] [diff] [review]
Add Image Rescaling for ThebesLayerOGL

looks OK, but why is it needed here?
Attachment #491750 - Flags: review?(roc) → review+
(In reply to comment #65)
> Comment on attachment 491750 [details] [diff] [review]
> Add Image Rescaling for ThebesLayerOGL
> 
> looks OK, but why is it needed here?

For the same reason as in BasicLayers? The retained content will be at the original size, and will look bad when scaled up.

Separating Images into their own layers fixed the specified test case, but an equivalent bug will exist with ThebesLayer content.
Attachment #492825 - Flags: review?(bas.schouten)
Is rounding out the rect to copy correct? This seems to work on OGL and when using GDI, but with d2d enabled it sometimes has 1pixel of black when scrolling.

Not sure if d2d is handling the scaled context slightly different somehow.
Attachment #492848 - Flags: review?(bas.schouten)
I need to think about the copy some more. Even if it does work I want to make sure I understand why and that it's correct.

I've only looked at the D3D10 case but some initial remarks:

- This could use some comments, it's all fairly magical what's happening and what coordinate systems things are in, documenting this will make it more maintainable.
- I'm seeing some mD3DManager->X/YResolution() calls I can't find these on m-c, where are these introduced?
- I'm pretty sure you need to set mXResolution and mYResolution on the ThebesLayer, right now I think it'll always consider the resolution changed!
(In reply to comment #70)
> I need to think about the copy some more. Even if it does work I want to make
> sure I understand why and that it's correct.
> 
> I've only looked at the D3D10 case but some initial remarks:
> 
> - This could use some comments, it's all fairly magical what's happening and
> what coordinate systems things are in, documenting this will make it more
> maintainable.

Sure, I'll add some relevant comments tomorrow.

> - I'm seeing some mD3DManager->X/YResolution() calls I can't find these on m-c,
> where are these introduced?

Currently these are part of BasicLayerManager, bug 601888 moves them onto LayerManager.

> - I'm pretty sure you need to set mXResolution and mYResolution on the
> ThebesLayer, right now I think it'll always consider the resolution changed!

I do! at the end of CreateNewTexture()
Added comments
Attachment #492848 - Attachment is obsolete: true
Attachment #493612 - Flags: review?(bas.schouten)
Attachment #492848 - Flags: review?(bas.schouten)
Added comments.
Attachment #492825 - Attachment is obsolete: true
Attachment #493613 - Flags: review?(bas.schouten)
Attachment #492825 - Flags: review?(bas.schouten)
I have some concern here, the main concern is that the region to draw should be pixel aligned on the destination surface. Not in the space that the region is in. The reason for this is that if I for example my resolution is 0.5, and I draw a rect at 0,0-3,3, it will end up being drawn to 0,0-1.5,1.5 then if I add another rect at 3,0-4,3 it will end up being drawn at 1.5,0-2,1.5 in a separate invalidation and a separate drawing call, the edge of these two rectangles would then have a seam since they're drawn with two drawing calls each getting 50% coverage.

If we redrew the entire column of pixels on the destination surface we should be fine though.
(In reply to comment #74)
> I have some concern here, the main concern is that the region to draw should be
> pixel aligned on the destination surface. Not in the space that the region is
> in. The reason for this is that if I for example my resolution is 0.5, and I
> draw a rect at 0,0-3,3, it will end up being drawn to 0,0-1.5,1.5 then if I add
> another rect at 3,0-4,3 it will end up being drawn at 1.5,0-2,1.5 in a separate
> invalidation and a separate drawing call, the edge of these two rectangles
> would then have a seam since they're drawn with two drawing calls each getting
> 50% coverage.

Wouldn't the first rect get snapped to 0,0 - 2,2 and the second rect be snapped to 2,0 - 2,2?

Maybe I'm misunderstanding though.
(In reply to comment #75)
> (In reply to comment #74)
> > I have some concern here, the main concern is that the region to draw should be
> > pixel aligned on the destination surface. Not in the space that the region is
> > in. The reason for this is that if I for example my resolution is 0.5, and I
> > draw a rect at 0,0-3,3, it will end up being drawn to 0,0-1.5,1.5 then if I add
> > another rect at 3,0-4,3 it will end up being drawn at 1.5,0-2,1.5 in a separate
> > invalidation and a separate drawing call, the edge of these two rectangles
> > would then have a seam since they're drawn with two drawing calls each getting
> > 50% coverage.
> 
> Wouldn't the first rect get snapped to 0,0 - 2,2 and the second rect be snapped
> to 2,0 - 2,2?
> 
> Maybe I'm misunderstanding though.

There's no reason for code drawing a rectangle to snap that right? We just transform by an 0.5 scale so cairo will just draw antialiased (unless it's drawn as a pixel snapped rectangle but I don't believe that's necessarily the case?) Maybe there's something in layout which guarantees it will pixel snap the RegionToDraw but I don't know of anything.
Hmm, I think I understand a little better now.

We should probably round aRegionToDraw out to pixel boundaries on the destination surface.
Matt, could you adjust the D3D9/D3D10 patches (and probably other codepaths already landed) to do this. They look good otherwise.
Matt, can you update this stuff? It needs to get landed :-)
Rebased against tip ready for landing
Attachment #489940 - Attachment is obsolete: true
Attachment #504547 - Flags: review+
This caused bustage because of incorrect rebasing.

Pushed a fix as http://hg.mozilla.org/mozilla-central/rev/0e2dde01a0fb
We discussed on IRC how to fix the issue that we want to clip to integer-aligned regions in both destination space and user space when scaling. One solution is to force the scale factors to always be N or 1/N for integer N. Then rounding out either the destination clip rects or the user-space clip rects will leave everything aligned.

Also, we should always round the resolution *up* so that content is always scaled *down* when compositing. This will minimize lossiness. (But we probably want to ensure that scale factors of N + epsilon are rounded down to N, in case of floating-point errors.)
Attached patch BasicLayers v7 (obsolete) — Splinter Review
Added snapping code, will update the other patches once this is reviewed.
Attachment #484568 - Attachment is obsolete: true
Attachment #508287 - Flags: review?(roc)
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attached patch Part 1: BasicLayers v8 (obsolete) — Splinter Review
Folded in the changes moving the ClampToScaleFactors helper to gfxUtils.

Makes the later patches in the queue cleaner.
Attachment #508287 - Attachment is obsolete: true
Attachment #508600 - Flags: review?(roc)
Attachment #508287 - Flags: review?(roc)
Separated this from another patch for better Fung Shui in my patch queue.

Marking all old patches as obsolete, will upload new ones as I complete them.
Attachment #484567 - Attachment is obsolete: true
Attachment #491749 - Attachment is obsolete: true
Attachment #491750 - Attachment is obsolete: true
Attachment #493612 - Attachment is obsolete: true
Attachment #493613 - Attachment is obsolete: true
Attachment #508602 - Flags: review?(roc)
Attachment #493612 - Flags: review?(bas.schouten)
Attachment #493613 - Flags: review?(bas.schouten)
+    gfxSize scale = aContext->CurrentMatrix().ScaleFactors(PR_TRUE);
+    float paintXRes = BasicManager()->XResolution() * ClampToScaleFactor(scale.width);
+    float paintYRes = BasicManager()->YResolution() * ClampToScaleFactor(scale.height);

I think you should multiply scale.width by XResolution() before passing to ClampToScaleFactor.

+  // Multipliers must be integers or 1/integer values.

this is not actually needed right? This function will work fine with any positive value.

+  NS_ASSERTION((IsFloatInteger(aXMult) || IsFloatInteger(1/aXMult)) &&
+               (IsFloatInteger(aYMult) || IsFloatInteger(1/aYMult)),
+               "Multiplication factors must be integers or 1/integer");

So take this out.

+  x = NSToCoordRound(NSToCoordFloor(float(x) * aXMult) / aXMult);
+  y = NSToCoordRound(NSToCoordFloor(float(y) * aYMult) / aYMult);
+  width = NSToCoordRound(NSToCoordCeil(float(width) * aXMult) / aXMult);
+  height = NSToCoordRound(NSToCoordCeil(float(height) * aYMult) / aYMult);

Hang on, if we're trying to get multiples of aXMult, this won't really work. I think you're getting multiples of 1/aXMult. So I suggest you flip this around to actually get multiples of aXMult, pass in 1/resolution when you call it, and add epsilons so that when 1/N doesn't quite evaluate perfectly, we don't jump up or down to the next pixel. 

Also I think x and y should be NSToCoordFloor on the outside, NSToCoordCeil for width/height, otherwise things are broken.
With the rest of the series, the other layer managers also support resolution changes.

Not sure if PresShell will ever set something other than 1.0 through this code path though. It may be unnecessary in this case.
In what situations would we need other layer managers to support resolution changes? I don't see a need for that right now.
(In reply to comment #87)
> +    gfxSize scale = aContext->CurrentMatrix().ScaleFactors(PR_TRUE);
> +    float paintXRes = BasicManager()->XResolution() *
> ClampToScaleFactor(scale.width);
> +    float paintYRes = BasicManager()->YResolution() *
> ClampToScaleFactor(scale.height);
> 
> I think you should multiply scale.width by XResolution() before passing to
> ClampToScaleFactor.
> 
> +  // Multipliers must be integers or 1/integer values.
> 
> this is not actually needed right? This function will work fine with any
> positive value.

I'm not sure about this at all.
Taking the rect (0,0,3,3) and scaling by (0.17, 0.17) <- arbitrary annoying numbers.

What should this be rounded out to such that it is integer values before and after scaling? The current algorithm comes up with (0,0,6,6), which isn't correct at all.

> +  x = NSToCoordRound(NSToCoordFloor(float(x) * aXMult) / aXMult);
> +  y = NSToCoordRound(NSToCoordFloor(float(y) * aYMult) / aYMult);
> +  width = NSToCoordRound(NSToCoordCeil(float(width) * aXMult) / aXMult);
> +  height = NSToCoordRound(NSToCoordCeil(float(height) * aYMult) / aYMult);
> 
> Hang on, if we're trying to get multiples of aXMult, this won't really work. I
> think you're getting multiples of 1/aXMult. So I suggest you flip this around
> to actually get multiples of aXMult, pass in 1/resolution when you call it, and
> add epsilons so that when 1/N doesn't quite evaluate perfectly, we don't jump
> up or down to the next pixel. 
> 
> Also I think x and y should be NSToCoordFloor on the outside, NSToCoordCeil for
> width/height, otherwise things are broken.

I'm not quite sure what the difference between x*xscale/xscale and x/(1/xscale)*(1/xscale) is other than potential rounding issues. My piece of paper calculations show the current algorithm coming up with correct values, but I've very likely missed an edge case, will think about this some more.
With the above example rect but instead scaling by (0.25, 0.25) <- Much nicer 1/integer values, the algorithm comes up with (0,0,4,4) which is what I'd expect.
(In reply to comment #91)
> I'm not quite sure what the difference between x*xscale/xscale and
> x/(1/xscale)*(1/xscale) is other than potential rounding issues.

Well sure, they're both 'x' once you get rid of floor/ceil :).

(In reply to comment #91)
> I'm not sure about this at all.
> Taking the rect (0,0,3,3) and scaling by (0.17, 0.17) <- arbitrary annoying
> numbers.
> 
> What should this be rounded out to such that it is integer values before and
> after scaling? The current algorithm comes up with (0,0,6,6), which isn't
> correct at all.

The comment for RoundOutToMultiple says "Snap rect outwards to integer multiples of aXMult/aYMult". It doesn't do that. Maybe it does what you want, but it doesn't do what it says on the label.

> > +  x = NSToCoordRound(NSToCoordFloor(float(x) * aXMult) / aXMult);
> > +  y = NSToCoordRound(NSToCoordFloor(float(y) * aYMult) / aYMult);
> > +  width = NSToCoordRound(NSToCoordCeil(float(width) * aXMult) / aXMult);
> > +  height = NSToCoordRound(NSToCoordCeil(float(height) * aYMult) / aYMult);

E.g. x = 3, aXMult = 2, we get x = round(floor(6) / 2) = 3. It hasn't been rounded to a multiple of 2.
Right, sorry, the comment/name are entirely wrong in every way :)

A better description might be:

Snaps the rect outwards such that the point/size values and their corresponding axis scales are paired factors of an integer.

Converting that to CamelCase probably won't be the most elegant function name, suggestions very much appreciated.
(In reply to comment #94)
> Snaps the rect outwards such that the point/size values and their corresponding
> axis scales are paired factors of an integer.

How about, "extend rect outwards so that the edges are on integer boundaries and the edges scaled by aXMult/aYMult are also in integer boundaries"?
Considering the code footprint touched here I believe this should be betan+ not final+.
Attached patch Part 1: BasicLayers v9 (obsolete) — Splinter Review
Hopefully better naming/description.

I guess part 2 is unnecessary, as discussed on irc. Will remove.
Attachment #508600 - Attachment is obsolete: true
Attachment #508965 - Flags: review?(roc)
Attachment #508600 - Flags: review?(roc)
Comment on attachment 508965 [details] [diff] [review]
Part 1: BasicLayers v9

+    gfxSize scale = aContext->CurrentMatrix().ScaleFactors(PR_TRUE);
+    float paintXRes = BasicManager()->XResolution() * gfxUtils::ClampToScaleFactor(scale.width);
+    float paintYRes = BasicManager()->YResolution() * gfxUtils::ClampToScaleFactor(scale.height);

> I think you should multiply scale.width by XResolution() before passing to
> ClampToScaleFactor.

+  // Extend the rect outwards such that the edges are on integer boundaries
+  // and the edges scaled by aXMult/aYMult are also on integer boundaries.

Document that aXMult/aYMult must be N or 1/N for integer N.

+static bool IsFloatInteger(float aFloat)
+{
+    return fabs(aFloat - NS_round(aFloat)) < 1e-6;
+}

Fix indent

Does ExtendForScaling actually work now? Hmm. If aXMult = N (N integer) then we get
  x' = floor(floor(x*N)/N)
     = x
which is an integer, so x'*aXMult is an integer too, OK.
If aXMult is 1/N (N integer) then we get
  x' = floor(floor(x/N)*N)
     = floor(k*N) where x=k*N + a (0 <= a < N, k integer)
     = k*N
which is an integer, and x'*aXMult = k which is an integer too. So the code does work.

If aXMult is an integer >= 1 then we don't have to do anything, because x and x*aXMult are integers already. Your code still works, but it might be more obvious to just write the conditional
  if (aXMult < 1) {
    width = NSToCoordRound(ceil(XMost() * aXMult) / aXMult);
    x = NSToCoordRound(floor(x * aXMult) / aXMult);
  }

Note that we shouldn't be rounding up width/height, but XMost/YMost. And we can avoid unnecessary int/float conversion by just using floor and ceil. It might also be worth commenting that the outer NSToCoordRound shouldn't change anything other than compensate for FP error; the inner expression should already be an integer.

r+ with all that
Attachment #508965 - Flags: review?(roc) → review+
Attached patch Part 1: BasicLayers v10 (obsolete) — Splinter Review
Carrying forward r=roc

As an note I've calculated the x value before width (opposite to your example) because the value of XMost() could change depending on how this rounds.

I've also added subtracting of x from the result to get back to a width instead of XMost() value.
Attachment #508965 - Attachment is obsolete: true
Attachment #509648 - Flags: review+
+    x = NSToCoordRound(floor(float(x) * aXMult) / aXMult);
+    width = NSToCoordRound(ceil(float(XMost()) * aXMult) / aXMult) - x;

I think you introduced a bug. XMost() in the second line will depend on the new value of x from the first line. That's why I did them in the other order :-)
Indeed I have :)
Attachment #509648 - Attachment is obsolete: true
Attachment #509660 - Flags: review+
Attachment #508602 - Attachment is obsolete: true
Attachment #508602 - Flags: review?(roc)
Attached patch Part 2: OpenGL implementation (obsolete) — Splinter Review
Attachment #509882 - Flags: review?(joe)
Attached patch Part 3: D3D9 implementation (obsolete) — Splinter Review
Attachment #509884 - Flags: review?(bas.schouten)
Attached patch Part 4: D3D10 Implementation (obsolete) — Splinter Review
Attachment #509886 - Flags: review?(bas.schouten)
Comment on attachment 509882 [details] [diff] [review]
Part 2: OpenGL implementation

>diff --git a/gfx/layers/opengl/ThebesLayerOGL.cpp b/gfx/layers/opengl/ThebesLayerOGL.cpp

>@@ -377,38 +383,56 @@ FillSurface(gfxASurface* aSurface, const

>+static nsIntSize
>+ScaledSize(const nsIntSize& aSize, float aXScale, float aYScale)
>+{
>+  if (aXScale == 1.0 && aYScale == 1.0) {
>+    return aSize;
>+  }
>+
>+  gfxRect rect(0, 0, aSize.width, aSize.height);
>+  rect.Scale(aXScale, aYScale);
>+  rect.RoundOut();

Can you just do rect.ScaleRoundOut(...)? And what about nsIntRect::ScaleRoundOut(), avoiding a fp<->int conversion?

>diff --git a/gfx/src/nsRect.h b/gfx/src/nsRect.h

>+  
>+  
>+  nsIntRect& ScaleRoundOut(float aXScale, float aYScale);

Extra whitespace here.

All the ScaleRoundOut changes should probably be put into their own changeset, because they're not *directly* related to the OpenGL changes.
Attachment #509882 - Flags: review?(joe) → review+
Fixed a bug with component alpha
Attachment #509884 - Attachment is obsolete: true
Attachment #510177 - Flags: review?(bas.schouten)
Attachment #509884 - Flags: review?(bas.schouten)
As above
Attachment #510177 - Attachment is obsolete: true
Attachment #510178 - Flags: review?(bas.schouten)
Attachment #510177 - Flags: review?(bas.schouten)
Attached patch Part 5: Test! (obsolete) — Splinter Review
Sent to try server again to confirm this test passes on all platforms.
Attachment #510179 - Flags: review?(roc)
Seems to me the test won't work, since we only create a layer for canvases that have a buffer, and we don't allocate the buffer until we draw into the canvas, so you need to add something to draw into the canvas.
Attached patch Part 5: Test! v2 (obsolete) — Splinter Review
This was relying on ContainerLayers remaining active even if the CanvasLayer wasn't created.

I've added drawing so that we don't need to rely on this behaviour.
Attachment #510179 - Attachment is obsolete: true
Attachment #510187 - Flags: review?(roc)
Attachment #510179 - Flags: review?(roc)
+ctx.fillStyle = "rgba(255, 255, 255, 1.0)";
+ctx.fillRect(0, 0, 1, 1);

You're filling with white. Shouldn't you give the <img> a white background, or a white image, to match?
Attached patch Part 5: Test! v3Splinter Review
New and improved, now with more white!
Attachment #510187 - Attachment is obsolete: true
Attachment #510188 - Flags: review?(roc)
Attachment #510187 - Flags: review?(roc)
ScaleRoundOut portion of part 2, review comments fixed.

Carrying forward r=joe
Attachment #509882 - Attachment is obsolete: true
Attachment #510228 - Flags: review+
Carrying forward r=joe
Attachment #510229 - Flags: review+
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs review]
Comment on attachment 510178 [details] [diff] [review]
Part 4: D3D10 Implementation v2

>diff --git a/gfx/layers/d3d10/ThebesLayerD3D10.h b/gfx/layers/d3d10/ThebesLayerD3D10.h
>--- a/gfx/layers/d3d10/ThebesLayerD3D10.h
>+++ b/gfx/layers/d3d10/ThebesLayerD3D10.h
>@@ -81,22 +81,33 @@ private:
> 
>   /* This contains the thebes surface */
>   nsRefPtr<gfxASurface> mD2DSurface;
> 
>   /* This contains the thebes surface for our render-on-white texture */
>   nsRefPtr<gfxASurface> mD2DSurfaceOnWhite;
> 
>   /* Have a region of our layer drawn */
>-  void DrawRegion(const nsIntRegion &aRegion, SurfaceMode aMode);
>+  void DrawRegion(nsIntRegion &aRegion, SurfaceMode aMode);
> 
>   /* Create a new texture */
>   void CreateNewTextures(const gfxIntSize &aSize, SurfaceMode aMode);
> 
>   /* Copy a texture region */
>   void CopyRegion(ID3D10Texture2D* aSrc, const nsIntPoint &aSrcOffset,
>                   ID3D10Texture2D* aDest, const nsIntPoint &aDestOffset,
>-                  const nsIntRegion &aCopyRegion, nsIntRegion* aValidRegion);
>+                  const nsIntRegion &aCopyRegion, nsIntRegion* aValidRegion,
>+                  float aXRes, float aYRes);
>+
>+  /**
>+   * Calculate the desired texture resolution based on
>+   * the layer managers resolution, and the current
>+   * transforms scale factor.
>+   */
>+  void GetDesiredResolutions(float& aXRes, float& aYRes);
>+
>+  /* Check if the current texture resolution matches */

Nit: matches the stores resolution
Attachment #510178 - Flags: review?(bas.schouten) → review+
Attachment #509886 - Attachment is obsolete: true
Attachment #509886 - Flags: review?(bas.schouten)
Attachment #510177 - Attachment is obsolete: false
Attachment #510177 - Flags: review?(bas.schouten)
Attachment #510177 - Flags: review?(bas.schouten) → review+
Whiteboard: [hardblocker][has patch][needs review] → [hardblocker][has patch]
Irc r=roc.

Just waiting on a local build to confirm this fixes the issue.

Looks like macs async image loading is causing problems with the empty image.
Attachment #510798 - Flags: review+
(In reply to comment #117)
> Created attachment 510798 [details] [diff] [review]
> Fix orange on Mac
> 
> Irc r=roc.
> 
> Just waiting on a local build to confirm this fixes the issue.
> 
> Looks like macs async image loading is causing problems with the empty image.

Landed as http://hg.mozilla.org/mozilla-central/rev/0625f07b31f5
Depends on: 633040
Depends on: 637180
Depends on: 641770
Depends on: 695275
The automated test for this bug is passing on all OSs (layout/reftests/bugs/586683-1.html, layout/reftests/bugs/586683-1-ref.html):
https://tbpl.mozilla.org/php/getParsedLog.php?id=7896783&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7948926&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7946462&full=1&branch=services-central.

The URL provided leads to a 404 error so the bug was only tested automatically.
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][qa!]
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.