Closed
Bug 994081
Opened 11 years ago
Closed 10 years ago
Convert imgFrame to store and act on a Moz2D SourceSurface instead of a Thebes gfxASurface
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jwatt, Assigned: mwu)
References
(Depends on 1 open bug)
Details
Attachments
(5 files, 14 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
In bug 950372 Bas started a patch to convert imgFrame to store and act on a Moz2D SourceSurface instead of a Thebes gfxASurface. I don't actually need that patch for bug 950372 though, so I'm spinning off this bug to continue work on that.
Reporter | ||
Comment 1•11 years ago
|
||
Oh, and this patch from bug 950372 too.
Assignee | ||
Comment 2•11 years ago
|
||
This seems to work for me. Going to run through try. I didn't have to add a moz2d draw to gfxDrawable, FWIW.
Assignee: nobody → mwu
Assignee | ||
Comment 4•11 years ago
|
||
Now with less orange.
Attachment #8409966 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
Hopefully with no orange now.
Attachment #8410697 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Some single color optimization issues fixed, hopefully.
Attachment #8411230 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Some general comments on what I did in this path -
A lot of the code is simplified because we stop caring about platform specific surfaces during decoding, and after decoding we simply store a reference to an optimized SourceSurface. Since we're just decompressing to DataSourceSurface, we don't have to worry about marking the surface as dirty or remembering if the format changed.
There's some perf issues I need to address after getting things green on try. The drawing path doesn't deal too well with plain DataSourceSurfaces, probably because it needs to generate a cairo/skia/etc surface wrapper every time.
Comment 9•11 years ago
|
||
Thanks so much for doing this, Michael. There's some refactoring I want to do in imagelib that will become much easier thanks to this work.
Assignee | ||
Comment 10•11 years ago
|
||
Store non-premultiplied colors, bypass mImageSurface->Map/Unmap when locking image data.
Attachment #8412202 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
This makes VolatileBufferPtrs more flexible by allowing them to use different VolatileBuffers as needed. Not very confident about the changes here, but they seem to work.
Attachment #8421191 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 12•11 years ago
|
||
This is a DataSourceSurface for VolatileBuffers. I used this instead of attaching data since we don't have to worry about keeping things synced with cairo surfaces and it seems like a more natural approach for C++ code.
Attachment #8421197 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 13•11 years ago
|
||
This passes all tests on try, but I think there's still perf regressions. I'm going to address perf regressions in other patches. There's also a bunch of weird things that I do in here to workaround bugs which I'll comment on.
Attachment #8413032 -
Attachment is obsolete: true
Attachment #8421198 -
Flags: review?(seth)
Assignee | ||
Comment 14•11 years ago
|
||
More context, and pick up some changes that should've been in this patch.
Attachment #8421198 -
Attachment is obsolete: true
Attachment #8421198 -
Flags: review?(seth)
Attachment #8421212 -
Flags: review?(seth)
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8421212 [details] [diff] [review]
Convert imgFrame to SourceSurfaces, v2
Review of attachment 8421212 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/src/RasterImage.cpp
@@ +864,5 @@
> + Factory::CreateDataSourceSurface(size, SurfaceFormat::B8G8R8A8);
> +
> + DataSourceSurface::MappedSurface mapping;
> + DebugOnly<bool> success =
> + surf->Map(DataSourceSurface::MapType::WRITE, &mapping);
We explicitly use a DataSourceSurface here since some users get confused if we give them something else.. I think DrawTargetCG or DrawTargetD2D gets confused, but I don't remember which one. We also explicitly use B8G8R8A8 because there's a test on WinXP which fails if we pass surface using SurfaceFormat::B8G8R8X8.
@@ +867,5 @@
> + DebugOnly<bool> success =
> + surf->Map(DataSourceSurface::MapType::WRITE, &mapping);
> + NS_ASSERTION(success, "Failed to map surface");
> + RefPtr<DrawTarget> target =
> + Factory::CreateDrawTargetForData(BackendType::CAIRO,
Cairo here and elsewhere is specifically used because reftests fail if we try to reconstruct single color optimized images using certain other backends.
::: image/src/imgFrame.cpp
@@ +192,5 @@
> mSinglePixel = true;
> + mSinglePixelColor.a = ((firstPixel >> 24) & 0xFF) * (1.0f / 255.0f);
> + mSinglePixelColor.r = ((firstPixel >> 16) & 0xFF) * (1.0f / 255.0f);
> + mSinglePixelColor.g = ((firstPixel >> 8) & 0xFF) * (1.0f / 255.0f);
> + mSinglePixelColor.b = ((firstPixel >> 0) & 0xFF) * (1.0f / 255.0f);
gfx::Color doesn't support taking ARGB or unpremultiplying colors, so it's all done manually here. I can move it over to gfx::Color if there could be more users.
Comment 16•11 years ago
|
||
Comment on attachment 8421191 [details] [diff] [review]
Make VolatileBufferPtrs more flexible
Review of attachment 8421191 [details] [diff] [review]:
-----------------------------------------------------------------
::: memory/mozalloc/VolatileBuffer.h
@@ +97,4 @@
> return mPurged;
> }
>
> + void Set(VolatileBuffer* vbuf) {
Do you need this to be public?
@@ +105,5 @@
> + mVBuf = vbuf;
> + mPurged = false;
> + if (vbuf) {
> + mPurged = !vbuf->Lock(&mMapping);
> + }
I'd rather DRY here, and have private functions that do the "heavy" lifting for the constructor and destructor, and have those functions called here. Or in the caller.
Attachment #8421191 -
Flags: review?(mh+mozilla) → feedback+
Comment 17•11 years ago
|
||
Comment on attachment 8421197 [details] [diff] [review]
Add SourceSurfaceVolatileBuffer
Moving review to bas
Attachment #8421197 -
Flags: review?(jmuizelaar) → review?(bas)
Comment 18•11 years ago
|
||
Comment on attachment 8421197 [details] [diff] [review]
Add SourceSurfaceVolatileBuffer
Review of attachment 8421197 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/2D.h
@@ +24,4 @@
> #include "mozilla/RefPtr.h"
>
> #include "mozilla/DebugOnly.h"
> +#include "mozilla/VolatileBuffer.h"
This is not part of MFBT so can't be used from Moz2D.
Attachment #8421197 -
Flags: review?(bas) → review-
Comment 19•11 years ago
|
||
Note if we want to do something based on the lifetime of the source surface, UserData can be used.
Assignee | ||
Comment 20•11 years ago
|
||
Changes made according to review comments.
Attachment #8421191 -
Attachment is obsolete: true
Attachment #8426581 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8421197 -
Attachment is obsolete: true
Attachment #8421212 -
Attachment is obsolete: true
Attachment #8421212 -
Flags: review?(seth)
Attachment #8426585 -
Flags: review?(seth)
Comment 22•11 years ago
|
||
Comment on attachment 8426581 [details] [diff] [review]
Make VolatileBufferPtrs more flexible, v2
Review of attachment 8426581 [details] [diff] [review]:
-----------------------------------------------------------------
::: memory/mozalloc/VolatileBuffer.h
@@ +133,5 @@
> + void operator =(VolatileBuffer* vbuf) {
> + Set(vbuf);
> + }
> +private:
> + VolatileBufferPtr(VolatileBufferPtr const& vbufptr);
MOZ_DELETE
Attachment #8426581 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 23•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=27ce3eadb6b6
Performance actually looks mostly fine. WinXP tsvgx actually speeds up for some reason. tsvgx and tp look fine elsewhere except for tsvgx on Linux where we're much slower. Wondering if we need to provide xlib surfaces in those cases.
Comment 24•11 years ago
|
||
That seems likely, tsvgx draws the same image repeatedly, and passing an image surface into xlib will do an upload. Quartz has a similar 'issue' where it will cache scaled copies of an image internally as long as we reuse the CGImage object. If we're drawing with a SourceSurfaceRawData each time then we lose that.
Assignee | ||
Comment 25•11 years ago
|
||
This fixes tsvgx perf on Linux.
Attachment #8427896 -
Flags: review?(bas)
Attachment #8427896 -
Flags: feedback?(karlt)
Comment 26•11 years ago
|
||
Comment on attachment 8427896 [details] [diff] [review]
Optimize surfaces on GTK/X11
Review of attachment 8427896 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/2d/DrawTargetCairo.cpp
@@ +1090,5 @@
> TemporaryRef<SourceSurface>
> DrawTargetCairo::OptimizeSourceSurface(SourceSurface *aSurface) const
> {
> +#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
> + if (!static_cast<gfxPlatformGtk*>(gfxPlatform::GetPlatform())->UseXRender()) {
We can't call into gfxPlatform here since Moz2D needs to build standalone (it's used for servo among other things).
We should just use cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_XLIB to decide if we should do this.
Assignee | ||
Comment 27•11 years ago
|
||
Thanks Matt. I've eliminated all dependencies on things inside gfx/thebes.
Attachment #8427896 -
Attachment is obsolete: true
Attachment #8427896 -
Flags: review?(bas)
Attachment #8427896 -
Flags: feedback?(karlt)
Attachment #8428074 -
Flags: review?(bas)
Attachment #8428074 -
Flags: feedback?(karlt)
Assignee | ||
Comment 28•11 years ago
|
||
Try run with all patches -
https://tbpl.mozilla.org/?tree=Try&rev=f58f694c00d1
Comment 29•11 years ago
|
||
Comment on attachment 8428074 [details] [diff] [review]
Optimize surfaces on GTK/X11, v2
>+#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
>+ if (cairo_surface_get_type(mSurface) != CAIRO_SURFACE_TYPE_XLIB) {
>+ return aSurface;
I wonder why MOZ_WIDGET_GTK is here. I expect MOZ_X11 is sufficient.
The code looks correct in this patch, but I suspect the logic is not quite right elsewhere.
The goal is to upload to the same type of surface as destination draw target.
If imgFrame doesn't want to decide how to optimize at Draw() time, and no
parameters are provided to indicate the destination surface type, then
Optimize() needs to guess the destination type.
ScreenReferenceDrawTarget() sounds like a target for drawing to a window, but
I assume imgFrames are usually drawn to thebes/content layers. If so, then
optimization should be to the surface type used for thebes/content layers,
rather than the type of the window. These are currently the same on X11 with
default prefs, but Skia Thebes layers are planned (e.g. bug 738937).
gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
as it was in gfxPlatform::OptimizeImage().
Attachment #8428074 -
Flags: feedback?(karlt) → feedback+
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> Comment on attachment 8428074 [details] [diff] [review]
> Optimize surfaces on GTK/X11, v2
>
> >+#if defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)
> >+ if (cairo_surface_get_type(mSurface) != CAIRO_SURFACE_TYPE_XLIB) {
> >+ return aSurface;
>
> I wonder why MOZ_WIDGET_GTK is here. I expect MOZ_X11 is sufficient.
>
GTK/GDK is being used to grab a Screen. I've updated the patch to grab it directly from mSurface.
> The code looks correct in this patch, but I suspect the logic is not quite
> right elsewhere.
>
> The goal is to upload to the same type of surface as destination draw target.
> If imgFrame doesn't want to decide how to optimize at Draw() time, and no
> parameters are provided to indicate the destination surface type, then
> Optimize() needs to guess the destination type.
>
Yeah, that's definitely an issue, especially when we're on platforms where we use different backends for content and canvas. I'm just trying to port imgFrame to moz2d without changing too much of the logic.
> ScreenReferenceDrawTarget() sounds like a target for drawing to a window, but
> I assume imgFrames are usually drawn to thebes/content layers. If so, then
> optimization should be to the surface type used for thebes/content layers,
> rather than the type of the window. These are currently the same on X11 with
> default prefs, but Skia Thebes layers are planned (e.g. bug 738937).
>
ScreenReferenceDrawTarget is actually for content - it's generated by gfxPlatform::CreateOffscreenContentDrawTarget.
> gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
> as it was in gfxPlatform::OptimizeImage().
I did that originally, but according to comment 26, that can't be checked. We could hack it into imgFrame::Optimize, I guess..
Assignee | ||
Comment 31•11 years ago
|
||
Dependency on GTK/GDK eliminated.
Attachment #8428074 -
Attachment is obsolete: true
Attachment #8428074 -
Flags: review?(bas)
Attachment #8428801 -
Flags: review?(bas)
Comment 32•11 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #30)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> > ScreenReferenceDrawTarget() sounds like a target for drawing to a window,
> ScreenReferenceDrawTarget is actually for content - it's generated by
> gfxPlatform::CreateOffscreenContentDrawTarget.
Oh, thanks. This should be good (or at least as good as the previous code) then.
Assignee | ||
Comment 33•10 years ago
|
||
Unbitrotted, added MOZ_DELETE.
Attachment #8426581 -
Attachment is obsolete: true
Comment 34•10 years ago
|
||
Comment on attachment 8426585 [details] [diff] [review]
Convert imgFrame to SourceSurfaces, v3
Review of attachment 8426585 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me!
(Also looks like I've got some rebasing ahead of me. =)
::: image/src/imgFrame.cpp
@@ +62,2 @@
> {
> + long stride = size.width * BytesPerPixel(format);
Why |long stride| here, but |int32_t stride| on line 48, for the result of the same expression?
@@ +206,5 @@
> /* Figure out if the entire image is a constant color */
>
> // this should always be true
> if (mImageSurface->Stride() == mSize.width * 4) {
> + uint32_t *imgData = (uint32_t*) ((uint8_t *)mVBufPtr);
Why the double cast?
Attachment #8426585 -
Flags: review?(seth) → review+
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #34)
> ::: image/src/imgFrame.cpp
> @@ +62,2 @@
> > {
> > + long stride = size.width * BytesPerPixel(format);
>
> Why |long stride| here, but |int32_t stride| on line 48, for the result of
> the same expression?
>
No idea. I'll make them consistent.
> @@ +206,5 @@
> > /* Figure out if the entire image is a constant color */
> >
> > // this should always be true
> > if (mImageSurface->Stride() == mSize.width * 4) {
> > + uint32_t *imgData = (uint32_t*) ((uint8_t *)mVBufPtr);
>
> Why the double cast?
The first cast is to get the pointer out of mVBufPtr, and then another cast to actually cast it to the right type. I guess VolatileBufferPtrs should have a get() method for this scenario.
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #29)
> gfxPlatformGtk::UseXRender() should be involved in the logic somewhere
> as it was in gfxPlatform::OptimizeImage().
Now that I think about it - I think we actually have this implicitly. The offscreen surface that we check against is generated by gfxPlatformGtk::CreateOffscreenSurface, which doesn't generate an XLib surface if we're not using XRender. The set of patches here should be enough.
Comment 37•10 years ago
|
||
Yes, I misunderstood "ScreenReference".
Updated•10 years ago
|
Attachment #8428801 -
Flags: review?(bas) → review+
Assignee | ||
Comment 38•10 years ago
|
||
Assignee | ||
Comment 39•10 years ago
|
||
Linux x64 ASAN fails a scaling reftest. Don't think those tests were running last time I sent these patches to try. I'm guessing that the high quality scaling path in RasterImage expects a zeroed dst buffer and the memory poisoning with ASAN breaks that assumption.
Reporter | ||
Comment 40•10 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #39)
> I'm guessing that the high quality
> scaling path in RasterImage expects a zeroed dst buffer and the memory
> poisoning with ASAN breaks that assumption.
Sounds like you should include "mozilla/gfx/DataSurfaceHelpers.h" and clear the surface with ClearDataSourceSurface. (That function was only just landed.)
Assignee | ||
Comment 41•10 years ago
|
||
It was actually the image deoptimization path in LockImageData causing problems. High quality image scaling triggers image deoptimization. That code draws with OP_OVER, but we can switch it to OP_SOURCE so the poisoned memory has no effect.
Assignee | ||
Comment 42•10 years ago
|
||
Unbitrotted, image deoptimization uses OP_SOURCE where necessary.
Attachment #8426585 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c16ab7d8eef
https://hg.mozilla.org/mozilla-central/rev/013456bbf74d
https://hg.mozilla.org/mozilla-central/rev/79624417d247
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 45•10 years ago
|
||
This increased Tp5 XRes by about 2.5mb, which is as expected since it moves images to the x server. No corresponding drop is visible in RSS, but the RSS numbers are too noisy to really see a 2.5mb drop.
Customization animation on OSX 10.6 takes 10% longer. There also seems to be a very slight impact on OSX 10.8. On the other hand, customization animation on Windows may have improved.
You need to log in
before you can comment on or make changes to this bug.
Description
•