Closed
Bug 579276
Opened 14 years ago
Closed 14 years ago
White text on a fixed position background triggers Bug 363861 (bad ClearType) with GDI and retained layers
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: the.decryptor, Assigned: roc)
References
Details
(Keywords: regression)
Attachments
(17 files, 3 obsolete files)
User-Agent: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre
As the summary says, white text on a fixed position background image (but not solid colour fill) causes the text to lose it's sub-pixel anti-aliasing, but when using GDI instead of D2D this triggers Bug 363861.
I'll upload a testcase, but you can also reproduce it by scrolling the awesomebar results, or this cssZenGarden page.
http://csszengarden.com/?cssfile=http://www.mnwebfolios.com/schaefer/awards/styleAwardnight.css
Reproducible: Always
Sorry, I just noticed the page I attached works ok when loaded in an iframe (but shows the bug when scrolling), but it shows the problem when loaded normally.
Comment 3•14 years ago
|
||
Comment 4•14 years ago
|
||
[STR]
1. Start Latest hourly Minefield (Landed retained layers)
http://hg.mozilla.org/mozilla-central/rev/7124132f0506
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre ID:20100715211042
2. Open Testcase
3. Zoom-In (if necessary)
[Actual]
Color of text is ugly.
Updated•14 years ago
|
Blocks: 564991
Keywords: regression
Updated•14 years ago
|
Summary: White text on a fixed position background triggers Bug 363861 with retained layers → White text on a fixed position background triggers Bug 363861 (bad ClearType) with GDI and retained layers
Assignee | ||
Comment 5•14 years ago
|
||
Thanks Alice.
This is sort-of expected; the fixed-pos content goes in its own layer, and the layer is transparent, so bug 363861 is triggered.
We can mitigate it by
Assignee | ||
Comment 6•14 years ago
|
||
oops.
We can mitigate this in some cases --- where the transparent layer is over a solid-color part of another layer --- by copying that solid color into the transparent layer, making it opaque again. But in general we are going to have to address 363861, and make text in transparent surfaces look less awful on Windows. It seems to me subpixel AA should be turned off in transparent surfaces, but that's not happening on Windows in some cases?
Depends on: 363861
Comment 7•14 years ago
|
||
Attachment #457792 -
Attachment is obsolete: true
Comment 8•14 years ago
|
||
I confirm the given problem happens on Windows XP using:
2010-07-16-04-mozilla-central/firefox-4.0b2pre.en-US.win32.zip
The previous day nightly is o.k.:
2010-07-15-04-mozilla-central/firefox-4.0b2pre.en-US.win32.zip
Comment 9•14 years ago
|
||
Another testcase:
1. Load http://www.w3.org/TR/SVGFilterPrimer12/
2. Note the rendering of "Erik Dahlström" on Windows XP w/ Cleartype enabled
- Firefox 3.6 = Cleartype anti-aliasing
- FF4beta/trunk = grayscale anti-aliasing
Updated•14 years ago
|
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 10•14 years ago
|
||
Here is the basic plan for dealing with subpixel AA and retained layers:
https://wiki.mozilla.org/Gecko:TransparentLayersAndSubpixelAA
I have patches implementing the first three steps of the plan.
Assignee | ||
Comment 11•14 years ago
|
||
This patch actually addresses two issues:
1) Different layer clients want to store different types of values as userdata. We can support this safely by storing a key/value pair instead of just a value. The key identifies which client stored the data, and that way clients can safely make assumptions about what type the data is.
2) We need to clean up userdata when the layer/layermanager goes away. This patch does that by forcing userdata values to descend from a base class with a virtual destructor.
Assignee: nobody → roc
Attachment #468577 -
Flags: superreview?(vladimir)
Attachment #468577 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #468577 -
Flags: review? → review?(jones.chris.g)
Assignee | ||
Comment 12•14 years ago
|
||
This is part one of the wiki plan. This is actually fairly straightforward to do.
Somehow an unrelated bug fix crept in here. We need to call RemoveDisplayItemDataForFrame for all frames when a LayerManagerData goes away, otherwise we don't be able to find those frames again when FrameLayerBuilder runs again.
Attachment #468578 -
Flags: review?(tnikkel)
Assignee | ||
Comment 13•14 years ago
|
||
Instead of adding lots of accessors, it's easier to use a flags word. This is also easier to copy around in shadowlayers.
I'm introducing two new flags here, CONTENT_NO_TEXT and CONTENT_NO_TEXT_OVER_TRANSPARENT. Hopefully the meaning is clear enough from the comments.
Attachment #468580 -
Flags: superreview?(vladimir)
Attachment #468580 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 14•14 years ago
|
||
Instead of extracting everything from nsDisplayItem* and passing it through various functions down to Accumulate, just pass the nsDisplayItem* down. This makes it easier to add functionality to Accumulate.
Attachment #468581 -
Flags: review?(tnikkel)
Assignee | ||
Comment 15•14 years ago
|
||
This is now very easy.
Attachment #468582 -
Flags: review?(tnikkel)
Assignee | ||
Comment 16•14 years ago
|
||
This is a good testcase for checking how well each platform can composite text into a transparent surface.
The first line of each block is the reference rendering. This is how the text should look.
The second line of each block is the rendering when the text is composited into a transparent surface. This will typically look different to the first line (it would only look the same as the first line if either the platform isn't doing subpixel AA, or the platform supports component alpha, or we do some tricky stuff to composite the background into the transparent surface before drawing the text on top of it).
The third line of each block is the rendering when the text is composited into a transparent surface, but the area inside the black box is filled with opaque white before the text is painted on top. One might hope that the part of the text over the box looks equivalent to the corresponding text on the first line.
Assignee | ||
Comment 17•14 years ago
|
||
We can classify the results of the second line and the third line (over the box) into three categories: "good" (text looks as good as the first line), "bad" (text looks OK but subpixel AA is lost or similar quality reduction), or "hideous" (bug 363861 or something similar is killing us).
Results for Windows:
Windows XP, Cleartype disabled: no antialiasing anywhere, so all "good".
Windows XP and 7, Cleartype enabled: second line "hideous", third line box "good"
Windows 7, DirectWrite enabled, both GDI and D2D: second line "bad", third line box "bad"
Assignee | ||
Comment 18•14 years ago
|
||
Results for Karl's Linux box: second line "bad", third line "good"
Results for my Macos 10.5 laptop: second line "bad", third line "good"
Assignee | ||
Comment 19•14 years ago
|
||
This patch creates an API that lets us expose knowledge about the platform font renderers as revealed by the previous testcase. We don't distinguish the "bad" and "hideous" classes, since we want to avoid both. Hopefully the comments are clear.
Currently no GetTextQualityInTransparentSurfaces implementation returns TEXT_QUALITY_OK, although on XP and Linux it might be worth checking if subpixel AA is enabled at all. If it isn't, then we can return TEXT_QUALITY_OK (meaning that transparent surfaces work just as well as opaque surfaces).
Attachment #468599 -
Flags: superreview?(vladimir)
Attachment #468599 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 20•14 years ago
|
||
Implements the third step of the wiki plan. Should be fairly straightforward.
Attachment #468600 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 21•14 years ago
|
||
Now that we are aggressively trying to get text to look great even in the presence of extra layers, we can bump the scroll timeout to a more reasonable value to reduce creation and destruction of layers.
Attachment #468603 -
Flags: review?(tnikkel)
Assignee | ||
Comment 22•14 years ago
|
||
The marquee test changes in bug 573933 are no longer needed. The text in marquees always gets subpixel AA again.
Assignee | ||
Comment 23•14 years ago
|
||
Ensure that text looks good when scrolled in an overflow:auto div and when it's over a background-attachment:fixed background.
Assignee | ||
Updated•14 years ago
|
blocking2.0: ? → beta6+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review]
Attachment #468599 -
Flags: superreview?(vladimir) → superreview+
Comment 24•14 years ago
|
||
Comment on attachment 468599 [details] [diff] [review]
Part 6: Add gfxASurface::GetTextQualityInTransparentSurfaces API
>+ /**
>+ * Determine how well text would be rendered in transparent surfaces that
>+ * are similar to this surface.
>+ */
>+ virtual TextQuality GetTextQualityInTransparentSurfaces()
>+ {
>+ return TEXT_QUALITY_BAD;
> }
I wondered if this should be pure virtual. It's not really an inheritable quality.
Also, is D2D intentionally TEXT_QUALITY_BAD? If so it may be better to override
it with a comment as to why.
Attachment #468599 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 25•14 years ago
|
||
(In reply to comment #24)
> >+ /**
> >+ * Determine how well text would be rendered in transparent surfaces that
> >+ * are similar to this surface.
> >+ */
> >+ virtual TextQuality GetTextQualityInTransparentSurfaces()
> >+ {
> >+ return TEXT_QUALITY_BAD;
> > }
>
> I wondered if this should be pure virtual. It's not really an inheritable
> quality.
I think making the default implementation BAD is a conservative and reasonable thing to do.
> Also, is D2D intentionally TEXT_QUALITY_BAD? If so it may be better to override
> it with a comment as to why.
Yes, it's BAD because it always renders grayscale AA to transparent surfaces, even over opaque pixels. I'll do as you suggest.
Comment on attachment 468577 [details] [diff] [review]
Part 1: make layer userdata APIs be key/value based
>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>+ /**
>+ * This setter can be used anytime. The user data for all keys is
>+ * initially null. Ownership pases to the layer manager.
>+ */
>+ void SetUserData(void* aKey, LayerUserData* aData)
>+ {
>+ NS_ASSERTION(!mUserDataKey || mUserDataKey == aKey,
>+ "Multiple LayerUserData objects not supported");
>+ mUserDataKey = aKey;
>+ mUserData = aData;
>+ }
>+ /**
>+ * This can be used anytime. Ownership passes to the caller!
>+ */
>+ nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
>+ {
>+ if (mUserDataKey == aKey) {
>+ // the nsAutoPtr constructor steals from mUserData
>+ mUserDataKey = nsnull;
>+ nsAutoPtr<LayerUserData> d(mUserData);
>+ return d;
>+ }
>+ nsAutoPtr<LayerUserData> d;
>+ return d;
>+ }
Since you already have the |HasUserData()| test, why not
|assert(!mUserDataKey || mUserDataKey == aKey)| here and in
GetUserData() too?
>+ /**
>+ * This getter can be used anytime.
>+ */
>+ PRBool HasUserData(void* aKey)
>+ {
>+ return mUserDataKey == aKey;
>+ }
>+ /**
>+ * This getter can be used anytime. Ownership is retained by the layer
>+ * manager.
>+ */
>+ LayerUserData* GetUserData(void* aKey)
>+ {
>+ return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
>+ }
>
I don't know why you're static_cast'ing null here.
>- // This setter and getter can be used anytime. The user data is initially
>- // null.
>- void SetUserData(void* aData) { mUserData = aData; }
>- void* GetUserData() { return mUserData; }
>+ /**
>+ * This setter can be used anytime. The user data for all keys is
>+ * initially null. Ownership pases to the layer manager.
>+ */
>+ void SetUserData(void* aKey, LayerUserData* aData)
>+ {
>+ NS_ASSERTION(!mUserDataKey || mUserDataKey == aKey,
>+ "Multiple LayerUserData objects not supported");
>+ mUserDataKey = aKey;
>+ mUserData = aData;
>+ }
>+ /**
>+ * This can be used anytime. Ownership passes to the caller!
>+ */
>+ nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
>+ {
>+ if (mUserDataKey == aKey) {
>+ // the nsAutoPtr constructor steals from mUserData
>+ mUserDataKey = nsnull;
>+ nsAutoPtr<LayerUserData> d(mUserData);
>+ return d;
>+ }
>+ nsAutoPtr<LayerUserData> d;
>+ return d;
>+ }
>+ /**
>+ * This getter can be used anytime.
>+ */
>+ PRBool HasUserData(void* aKey)
>+ {
>+ return mUserDataKey == aKey;
>+ }
>+ /**
>+ * This getter can be used anytime. Ownership is retained by the layer
>+ * manager.
>+ */
>+ LayerUserData* GetUserData(void* aKey)
>+ {
>+ return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
>+ }
>
It sucks to duplicate this code, since odds are we'll end up adding a
multi-key interface. Instead, why not factor this out into
class HasLayerUserData {
void SetUserData(void* aKey, LayerUserData* aData)
...
protected:
nsAutoPtr<LayerUserData> mUserData;
}
and then
class LayerManager : protected HasLayerUserData {
public:
using HasLayerUserData::SetUserData;
...
class Layer : protected HasLayerUserData {
public:
using HasLayerUserData::SetUserData;
...
>diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp
I only skimmed from here on down and above the Layers.h changes.
Comment on attachment 468580 [details] [diff] [review]
Part 3: Change Set/IsOpaqueContent API to Get/SetContentFlags
>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>+ enum {
>+ /**
>+ * If this is set, the caller is promising that by the end of this
>+ * transaction the entire visible region (as specified by
>+ * SetVisibleRegion) will be filled with opaque content.
>+ */
>+ CONTENT_OPAQUE = 0x01,
>+ /**
>+ * ThebesLayers only!
>+ * If this is set, the caller is promising that the visible region
>+ * contains no text at all. If this is set,
>+ * CONTENT_NO_TEXT_OVER_TRANSPARENT will also be set.
>+ */
>+ CONTENT_NO_TEXT = 0x02,
>+ /**
>+ * ThebesLayers only!
>+ * If this is set, the caller is promising that the visible region
>+ * contains no text over transparent pixels (any text, if present,
>+ * is over fully opaque pixels).
>+ */
>+ CONTENT_NO_TEXT_OVER_TRANSPARENT = 0x04
>+ };
Why not separate flags for ThebesLayers then? Layer attributes are
easy to remote for shadow layers, no worries about that.
OK by me either way.
Attachment #468580 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 468600 [details] [diff] [review]
Part 7: Don't retain transparent layers that would hurt text rendering
>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
> void
> BasicThebesLayer::Paint(gfxContext* aContext,
> LayerManager::DrawThebesLayerCallback aCallback,
> void* aCallbackData,
> float aOpacity)
> {
> NS_ASSERTION(BasicManager()->InDrawing(),
> "Can only draw in drawing phase");
> gfxContext* target = BasicManager()->GetTarget();
> NS_ASSERTION(target, "We shouldn't be called if there's no target");
>+ nsRefPtr<gfxASurface> targetSurface = aContext->CurrentSurface();
>
>- if (!BasicManager()->IsRetained()) {
>+ PRBool canUseOpaqueSurface = CanUseOpaqueSurface();
>+ if (!BasicManager()->IsRetained() ||
>+ (aOpacity == 1.0 && !canUseOpaqueSurface &&
>+ !ShouldRetainTransparentSurface(mContentFlags, targetSurface))) {
>+ mValidRegion.SetEmpty();
>+ mBuffer.Clear();
>+
> if (aOpacity != 1.0) {
> target->Save();
> ClipToContain(target, mVisibleRegion.GetBounds());
> target->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> }
>- mValidRegion.SetEmpty();
>- mBuffer.Clear();
> aCallback(this, target, mVisibleRegion, nsIntRegion(), aCallbackData);
> if (aOpacity != 1.0) {
> target->PopGroupToSource();
> target->Paint(aOpacity);
> target->Restore();
> }
> return;
> }
>
>- nsRefPtr<gfxASurface> targetSurface = aContext->CurrentSurface();
>- PRBool isOpaqueContent =
>- (targetSurface->AreSimilarSurfacesSensitiveToContentType() &&
>- aOpacity == 1.0 &&
>- CanUseOpaqueSurface());
> {
>+ PRBool opaqueBuffer = canUseOpaqueSurface &&
>+ targetSurface->AreSimilarSurfacesSensitiveToContentType();
> Buffer::ContentType contentType =
>- isOpaqueContent ? gfxASurface::CONTENT_COLOR :
>- gfxASurface::CONTENT_COLOR_ALPHA;
>+ opaqueBuffer ? gfxASurface::CONTENT_COLOR :
>+ gfxASurface::CONTENT_COLOR_ALPHA;
It appears to me that we'll end up creating an opaque surface even if
we're painting with opacity < 1.0, which isn't intended, right?
Assignee | ||
Comment 29•14 years ago
|
||
(In reply to comment #26)
> Comment on attachment 468577 [details] [diff] [review]
> Part 1: make layer userdata APIs be key/value based
> >+ * This can be used anytime. Ownership passes to the caller!
> >+ */
> >+ nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
> >+ {
> >+ if (mUserDataKey == aKey) {
> >+ // the nsAutoPtr constructor steals from mUserData
> >+ mUserDataKey = nsnull;
> >+ nsAutoPtr<LayerUserData> d(mUserData);
> >+ return d;
> >+ }
> >+ nsAutoPtr<LayerUserData> d;
> >+ return d;
> >+ }
>
> Since you already have the |HasUserData()| test, why not
> |assert(!mUserDataKey || mUserDataKey == aKey)| here and in
> GetUserData() too?
Because that forces the caller to always call HasUserData(). It seems simpler for callers to get null instead of doing that.
> >+ /**
> >+ * This getter can be used anytime.
> >+ */
> >+ PRBool HasUserData(void* aKey)
> >+ {
> >+ return mUserDataKey == aKey;
> >+ }
> >+ /**
> >+ * This getter can be used anytime. Ownership is retained by the layer
> >+ * manager.
> >+ */
> >+ LayerUserData* GetUserData(void* aKey)
> >+ {
> >+ return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
> >+ }
> >
>
> I don't know why you're static_cast'ing null here.
mUserData is not a pointer type so the compiler gets confused. But yeah, I can just use mUserData.get() instead. I'll do that.
> It sucks to duplicate this code, since odds are we'll end up adding a
> multi-key interface. Instead, why not factor this out into
>
> class HasLayerUserData {
> void SetUserData(void* aKey, LayerUserData* aData)
> ...
> protected:
> nsAutoPtr<LayerUserData> mUserData;
> }
>
> and then
>
> class LayerManager : protected HasLayerUserData {
> public:
> using HasLayerUserData::SetUserData;
> ...
>
> class Layer : protected HasLayerUserData {
> public:
> using HasLayerUserData::SetUserData;
> ...
Instead of protected inheritance, would you be OK with a direct member and forwarding of the Set/Get/Has/Remove calls to the member? And how about LayerUserDataSet instead of HasLayerUserData?
Assignee | ||
Comment 30•14 years ago
|
||
(In reply to comment #27)
> Why not separate flags for ThebesLayers then? Layer attributes are
> easy to remote for shadow layers, no worries about that.
So we don't need a separate flags word and separate accessor functions. That's all.
Assignee | ||
Comment 31•14 years ago
|
||
(In reply to comment #28)
> It appears to me that we'll end up creating an opaque surface even if
> we're painting with opacity < 1.0, which isn't intended, right?
Sorry. That change was intended, and I should have mentioned it. That change is perfectly OK, cairo can paint an opaque surface with a global opacity factor applied, no problem.
(In reply to comment #29)
> (In reply to comment #26)
> > >+ /**
> > >+ * This getter can be used anytime.
> > >+ */
> > >+ PRBool HasUserData(void* aKey)
> > >+ {
> > >+ return mUserDataKey == aKey;
> > >+ }
> > >+ /**
> > >+ * This getter can be used anytime. Ownership is retained by the layer
> > >+ * manager.
> > >+ */
> > >+ LayerUserData* GetUserData(void* aKey)
> > >+ {
> > >+ return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
> > >+ }
> > >
> >
> > I don't know why you're static_cast'ing null here.
>
> mUserData is not a pointer type so the compiler gets confused. But yeah, I can
> just use mUserData.get() instead. I'll do that.
>
Duh, right. OK.
> > It sucks to duplicate this code, since odds are we'll end up adding a
> > multi-key interface. Instead, why not factor this out into
> >
> > class HasLayerUserData {
> > void SetUserData(void* aKey, LayerUserData* aData)
> > ...
> > protected:
> > nsAutoPtr<LayerUserData> mUserData;
> > }
> >
> > and then
> >
> > class LayerManager : protected HasLayerUserData {
> > public:
> > using HasLayerUserData::SetUserData;
> > ...
> >
> > class Layer : protected HasLayerUserData {
> > public:
> > using HasLayerUserData::SetUserData;
> > ...
>
> Instead of protected inheritance, would you be OK with a direct member and
> forwarding of the Set/Get/Has/Remove calls to the member? And how about
> LayerUserDataSet instead of HasLayerUserData?
Sure.
Updated•14 years ago
|
Attachment #468577 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 468600 [details] [diff] [review]
Part 7: Don't retain transparent layers that would hurt text rendering
(In reply to comment #31)
> (In reply to comment #28)
> > It appears to me that we'll end up creating an opaque surface even if
> > we're painting with opacity < 1.0, which isn't intended, right?
>
> Sorry. That change was intended, and I should have mentioned it. That change is
> perfectly OK, cairo can paint an opaque surface with a global opacity factor
> applied, no problem.
Interesting. This might be a slight perf win then, right?
Attachment #468600 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 34•14 years ago
|
||
Yes. It actually fixes a regression that landed with retained layers (which didn't seem to be picked up in any bug).
Comment 35•14 years ago
|
||
Comment on attachment 468578 [details] [diff] [review]
Part 2: for a transparent layer over a solid color, hoist the color into the background of the layer
@@ -960,17 +1037,17 @@ ContainerState::ProcessDisplayItems(cons
- NS_ASSERTION(ownLayer->HasUserData(&gLayerManagerUserData),
+ NS_ASSERTION(!ownLayer->HasUserData(&gLayerManagerUserData),
Seems like this change should just be done correctly in part 1 instead of doing it wrong in part 1 and then fixing it in part 2.
Attachment #468578 -
Flags: review?(tnikkel) → review+
Comment 36•14 years ago
|
||
Comment on attachment 468581 [details] [diff] [review]
Part 4: Move nsDisplayItem parameter down to ThebesLayerData::Accumulate
@@ -164,20 +164,20 @@ protected:
+ void Accumulate(nsDisplayListBuilder* aBuilder,
+ nsDisplayItem* aItem,
+ const nsIntRect& aVisibleRect,
+ const nsIntRect& aDrawRect);
ContainerState stores an mBuilder in its members, so why bother passing a nsDisplayListBuilder argument to ContainerState::Accumulate?
Attachment #468581 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 37•14 years ago
|
||
Because Accumulate is a member of ThebesLayerData.
Comment 38•14 years ago
|
||
Oh, ok.
Comment 39•14 years ago
|
||
Comment on attachment 468582 [details] [diff] [review]
Part 5: Set CONTENT_NO_TEXT and CONTENT_NO_TEXT_OVER_TRANSPARENT flags
+ /**
+ * True if there is any text visible in the layer that's over
+ * transparent pixels.
+ */
+ PRPackedBool mHasTextOverTransparent;
I think the comment would be clearer if you added "in the layer" to the end.
+IsText(nsDisplayItem* aItem) {
Other display item types that might draw something we want to consider text:
TYPE_ALT_FEEDBACK (when drawing alt text for images)
TYPE_BULLET
TYPE_CARET (would the caret ever benefit from this, or is the caret rect always snapped nicely?)
TYPE_HEADER_FOOTER
TYPE_MATHML_CHAR_FOREGROUND
Of those I think bullet might be important.
Attachment #468582 -
Flags: review?(tnikkel) → review+
Comment 40•14 years ago
|
||
Comment on attachment 468603 [details] [diff] [review]
Part 8: Bump scroll timeout up a lot
Can you restore the "That's 4 generations of xms each." part of the comment? I think it's confusing otherwise.
Attachment #468603 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 41•14 years ago
|
||
(In reply to comment #39)
> Other display item types that might draw something we want to consider text:
> TYPE_ALT_FEEDBACK (when drawing alt text for images)
> TYPE_BULLET
Good call.
> TYPE_CARET (would the caret ever benefit from this, or is the caret rect always
> snapped nicely?)
No, it always snaps. Anyway it's just a rect, it doesn't get subpixel AA on any platform.
> TYPE_HEADER_FOOTER
> TYPE_MATHML_CHAR_FOREGROUND
Good call.
(In reply to comment #40)
> Comment on attachment 468603 [details] [diff] [review]
> Part 8: Bump scroll timeout up a lot
>
> Can you restore the "That's 4 generations of xms each." part of the comment? I
> think it's confusing otherwise.
Sure.
Comment 42•14 years ago
|
||
(In reply to comment #41)
> (In reply to comment #39)
> > Other display item types that might draw something we want to consider text:
> > TYPE_ALT_FEEDBACK (when drawing alt text for images)
> > TYPE_BULLET
>
> Good call.
If you're going to do these then you might want to consider that I think both of them can be text, or can just be images.
Attachment #468577 -
Flags: superreview?(vladimir) → superreview+
Comment on attachment 468580 [details] [diff] [review]
Part 3: Change Set/IsOpaqueContent API to Get/SetContentFlags
I sorta miss the days when we actually used fewer C++ things, like ref return values that lead to assignment to function calls...
Attachment #468580 -
Flags: superreview?(vladimir) → superreview+
Assignee | ||
Comment 44•14 years ago
|
||
(In reply to comment #42)
> If you're going to do these then you might want to consider that I think both
> of them can be text, or can just be images.
Hmm. I'll take ALT_FEEDBACK off the list --- no subpixel AA for it! I think we can live with that and it will avoid changing layer types when an image loads in an image-only layer.
For bullets, I think we don't need to optimize for cases where there are non-text bullets but no other text in the layer. So I'll leave that on the list.
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 45•14 years ago
|
||
I tried pushing this but it bounced. I think I fixed the problem but I'm rerunning through try server to make sure.
Whiteboard: [needs landing]
Assignee | ||
Comment 47•14 years ago
|
||
This is the real fix for the regression. As documented in Layers.h, DrawThebesLayer callbacks expect the caller to ensure that drawing outside the aRegionToDraw has no effect. We weren't doing that on the nonretained path.
Attachment #471480 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #471480 -
Flags: review? → review?(jones.chris.g)
Assignee | ||
Comment 48•14 years ago
|
||
As we did for the retained path, we should do for the nonretained path as well. This is unrelated to the regression, just a bonus.
Attachment #471481 -
Flags: review?(jones.chris.g)
Comment 49•14 years ago
|
||
I tried hourly with this bug (http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1283425322/firefox-4.0b6pre.en-US.win32.zip), but the performance of FX itself was a lot worse.
Attachment #471479 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 50•14 years ago
|
||
Performance doing what?
Updated•14 years ago
|
Attachment #471480 -
Flags: review?(jones.chris.g) → review+
Updated•14 years ago
|
Attachment #471481 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 51•14 years ago
|
||
Add a ClipToRegionSnapped to support snapping of the region rects.
Attachment #471666 -
Flags: review?(vladimir)
Assignee | ||
Comment 52•14 years ago
|
||
Call ClipToRegionSnapped on the non-retained path. This fixes a reftest failure reported by the tryserver ... a non-retained element transformed by a non-integer pixel offset was getting fuzzy edges because we got an unaligned clip.
Attachment #471479 -
Attachment is obsolete: true
Attachment #471480 -
Attachment is obsolete: true
Attachment #471669 -
Flags: review?(jones.chris.g)
Updated•14 years ago
|
Attachment #471669 -
Flags: review?(jones.chris.g) → review+
Comment 53•14 years ago
|
||
(In reply to comment #50)
> Performance doing what?
Tab animations, listing in menus (favs in bookmarks showing one by one)...
Comment 54•14 years ago
|
||
(In reply to comment #53)
> (In reply to comment #50)
> > Performance doing what?
>
> Tab animations, listing in menus (favs in bookmarks showing one by one)...
I can confirm all of that.
...
Not sure if related, today's builds are extremely slow w. D2D at scrolling & rendering. ?
Assignee | ||
Comment 55•14 years ago
|
||
All pages, or just some?
Assignee | ||
Comment 56•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c9298e3a5837
http://hg.mozilla.org/mozilla-central/rev/2c1237769e8f
http://hg.mozilla.org/mozilla-central/rev/83cbb8c8ead2
http://hg.mozilla.org/mozilla-central/rev/8b6202784029
http://hg.mozilla.org/mozilla-central/rev/a38c0e3a5c10
http://hg.mozilla.org/mozilla-central/rev/6cb2815eb8f4
http://hg.mozilla.org/mozilla-central/rev/b6788af16a8d
http://hg.mozilla.org/mozilla-central/rev/6854b075f70f
http://hg.mozilla.org/mozilla-central/rev/4021846f9188
http://hg.mozilla.org/mozilla-central/rev/2a3bda7f342e
http://hg.mozilla.org/mozilla-central/rev/e3d31ba6030c
http://hg.mozilla.org/mozilla-central/rev/c888cdffd617
http://hg.mozilla.org/mozilla-central/rev/6e70745e6665
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 57•14 years ago
|
||
(In reply to comment #55)
> All pages, or just some?
All pages. Performance issues w/o D2D as well.
Assignee | ||
Comment 58•14 years ago
|
||
I'll look at this regression in bug 593361.
Attachment #471666 -
Flags: review?(vladimir) → review+
Updated•14 years ago
|
Whiteboard: [needs landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•