Closed Bug 764299 Opened 12 years ago Closed 11 years ago

Cache rasterized SVG images some of the time

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: dholbert, Assigned: seth)

References

(Blocks 3 open bugs)

Details

(Keywords: perf, Whiteboard: [Australis:P1][Australis:M9])

Attachments

(4 files, 19 obsolete files)

(Part 1) - Add hashing to SVGImageContext.
2.56 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Part 3) - Add memory reporting to the surface cache.
5.98 KB, patch
Details | Diff | Splinter Review
(Part 4) - Cache rasterized surfaces in VectorImage.
15.62 KB, patch
Details | Diff | Splinter Review
(Part 2) - Add a temporary surface cache to imagelib.
26.26 KB, patch
Details | Diff | Splinter Review
Right now, each Draw operation on an SVG image triggers an actual re-rendering of (part of) the SVG document, via nsIPresShell::RenderDocument on the internal SVG document. We should cache that rasterized result, at least in simple cases where the SVG image is static (not animated) and is always drawn at the same resolution (only one client, or multiple clients all drawing it at the same size).
No longer blocks: australis-tabs-win
Is there an estimate on how long it would take to fix this bug and is this something that could be worked on soon? There's a fair chance we'll use SVG in bug 738491.
OS: Linux → All
Hardware: x86_64 → All
(In reply to Matthew N. [:MattN] from comment #1) > Is there an estimate on how long it would take to fix this bug Hard to say, but to establish some reasonable bounds, I'd say somewhere between 2 weeks and 2 months. > and is this > something that could be worked on soon? I don't have cycles for it right now, but I hopefully will in a few weeks. CC'ing roc and jet in case they know of anyone else who could take this sooner.
Thinking out loud: There are (at least) two ways we could take this. #1: We could treat the rasterized snapshot of an SVG file much like we treat the "decoded" version of other image files. Ideally we'd want to follow the same rules that other images-types do for their decoded incarnations. (how long to wait before discarding the rasterized "decoded" version, etc) ...or alternately... #2: We could store the rasterized SVG as a (compressed) PNG, which we'd keep around indefinitely and which would manage its own when-to-decode behavior just as if it were a normal PNG. Offhand, I don't really know how our decoded-image-caching logic works, so I'm not sure which of the above would be easier to shoehorn into it.
It wouldn't be *that* hard to hook into DiscardTracker et al, but it might be faster to rasterize to a PNG then redirect requests at that size to a LoadImage with that as a data URI. It probably makes more long-term sense to hook in to DiscardTracker, though.
Matthew, what parts of the UI are you considering using SVG for? How is the SVG to be integrated into the UI? Will it be inline in XUL markup, or embedded via an HTML <img> tag (or the XUL equivalent)?
(In reply to Jonathan Watt [:jwatt] from comment #5) > Matthew, what parts of the UI are you considering using SVG for? For Australis tabs (see bug 738491 and the patch there) > How is the SVG to be integrated into the UI? Will it be inline in XUL markup, or > embedded via an HTML <img> tag (or the XUL equivalent)? Three ways so far (see attachment 662848 [details] [diff] [review] for more context): * clip-path: url(chrome://browser/content/browser.xul#winstripe-tab-curve-clip-path); * content: url(chrome://browser/skin/tab-border-curve.svg); * background-image: url(chrome://browser/skin/tab-border-middle.svg), linear-gradient(to bottom, transparent, transparent 2px, hsla(0, 0%, 100%, 0.8) 2px, hsl(204, 33%, 97%) 2px, hsl(210, 58%, 95%));
How does this interact with high DPI rendering? If the window moves to a screen with a different DPI, will hooking into DiscardTracker be enough to ensure that the SVG gets re-rasterized?
The cache would have to account for varying resolution, not just to handle high-DPI, but also for SVG images rendered under varying transforms. Especially because the same image could appear at different resolutions simultaneously.
Sounds like it needs to be per-use (i.e. per-imgRequestProxy) and not per-Image.
It might be worth splitting this into a simpler case of repeating usage (such as Australis tabs/buttons borders) and dynamic usage (such as games where images might be rotating and resizing).
Is there anything different about the two cases, though? Seems like, given the concerns raised so far, the cache needs to be tied to particular |Image::Draw| parameters. If you don't get the same parameters as last time, you invalidate the cache. (Actually you don't need to invalidate for pure translations, but you get the idea.)
(In reply to Seth Fowler [:seth] from comment #11) > Is there anything different about the two cases, though? I considered the tie to draw parameters as the simpler case. If we want a single cached raster to also be used for different transformations as suggested in comment #8, then it cannot be tied to draw parameters, and also has other implications (probably cache big enough "base" raster etc).
(In reply to Avi Halachmi (:avih) from comment #12) > If we want a single cached raster to also be used for different transformations > as suggested in comment #8, then it cannot be tied to draw parameters, and also > has other implications (probably cache big enough "base" raster etc). Seems like what you're saying is that the different transformations will draw the image at different sizes, constantly invalidating the cache. As I think you're suggesting, maybe what we can do in this case is cache the largest size we've drawn the image at so far, handling smaller requests via scaling.
I don't think you should do that. I think you should make the scale factors of the transform at Draw() time part of the cache key.
When do we invalidate the various scaled versions of the image?
Waiting to hear if Jonathan can take this.
Flags: needinfo?(jwatt)
(In reply to Milan Sreckovic [:milan] from comment #17) > Waiting to hear if Jonathan can take this. I'll talk to Jet about this in our 1:1 on Wednesday. To set expectations, I have no experience of the svg-as-image stuff that dholbert implemented or of imagelib, so if dholbert estimates 2 weeks to 2 months for this (comment 2) then I'd be looking at that plus some I would expect. Leaving the needinfo?jwatt so I remember to report back here.
Flags: needinfo?(jwatt)
Keywords: perf
Flags: needinfo?(jwatt)
Jonathan, please talk to me before you dig much further. I have already implemented much of the machinery needed here for bug 859377. I'm aiming to get the patches involved ready for review today, and then we can probably trivially generalize the code to handle all SVGs.
Thanks for the heads up, Seth. And the reminder to comment here. It's going to be at least two weeks before I get to this.
Flags: needinfo?(jwatt)
I don't see you on IRC or else I'd be talking to you there, but actually I discussed this a bit more with dholbert, and I think I may end up just going ahead and implementing this if that's alright with you. The stuff I need for bug 859377 is so close to this that I may as well just do this too and kill two birds with one stone.
If you can take it on, by all means, that's fine by me. :)
Marking as depending on bug 859377, per comment 20 & comment 22.
Depends on: 859377
Seth, any update on what you might do with this bug?
Flags: needinfo?(seth)
I will start working on this bug and a couple of related bugs on Monday; this will be one of the things I hope to complete during the web rendering work week. I won't get to it before then, as I'm trying to get some things in a good state for interns who are starting next week while I'm out of town.
Flags: needinfo?(seth)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Blocks: 873551
Blocks: 821188
Blocks: 697502
Do we expect this cache to yield performance comparable to PNGs? We'd like to use SVG images for toolbar buttons on Linux in bug 875479. One reason is to pickup system colors e.g. -moz-DialogText. Do we have an idea yet of conditions that would cause SVG to not get cached? See the URL in that bug for examples of the SVG we'd like to use.
I expect this cache to yield performance comparable to PNGs in typical cases, with the exception that the initial rasterization involved in the creation of a cache entry will happen on the main thread, while PNGs can be decoded on a separate thread right now. There are things we can do to be more clever about that, but they won't be part of this initial patch. The main thing you'll want to do to maximize the benefits of the cache is to avoid drawing the SVG at many different, frequently changing sizes. Usually that's not a problem.
Attached patch (TEST) - Initial experiment. (obsolete) — — Splinter Review
This is an initial test to check how we handle rendering from the cache. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=32f06bc8be1c
Blocks: 886279
Attached patch (TEST) - Initial experiment. (obsolete) — — Splinter Review
Rebased against bug 885939 in preparation for making this more full-featured.
Attachment #766178 - Attachment is obsolete: true
Whiteboard: [Australis:P?][Australis:M?]
Any update on this? Marking as Australis:P1 since this is likely to get us some good perf wins on tab animation tests, which is our last blocker to landing.
Flags: needinfo?(seth)
Whiteboard: [Australis:P?][Australis:M?] → [Australis:P1][Australis:M?]
Jared, sorry I'm not updating this bug more frequently, but rest assured that I am working on this and there will be a patch up here in the next couple of days. I have a patch 90% done but I needed to put out some fires today and couldn't finish it up. Incidentally, how can I run the tab animation perf tests that you're running? I'd love to be able to get some numbers for this patch.
Flags: needinfo?(seth)
(In reply to Seth Fowler [:seth] from comment #32) > Jared, sorry I'm not updating this bug more frequently, but rest assured > that I am working on this and there will be a patch up here in the next > couple of days. I have a patch 90% done but I needed to put out some fires > today and couldn't finish it up. Great news, thank you for working on it! > Incidentally, how can I run the tab animation perf tests that you're > running? I'd love to be able to get some numbers for this patch. You can run it via tryserver using, "try: -b o -p linux,macosx64,win32 -u none -t svgr". The aggregate number will show up as 'tart' on TBPL. If you want to run it locally, you can install the add-on located at https://bugzilla.mozilla.org/attachment.cgi?id=794736 and then visit chrome://tart/content/tart.html. Unfortunately you will have to average the local numbers to compare your progress against the TBPL talos numbers.
(In reply to Seth Fowler [:seth] from comment #32) > Jared, sorry I'm not updating this bug more frequently, but rest assured > that I am working on this and there will be a patch up here in the next > couple of days. I have a patch 90% done but I needed to put out some fires > today and couldn't finish it up. > > Incidentally, how can I run the tab animation perf tests that you're > running? I'd love to be able to get some numbers for this patch. I'd be happy to run these numbers for you if you'd like. :) I can do that once you post your patch.
Attached patch (TEST) - Initial experiment. (obsolete) — — Splinter Review
I appreciate that, Mike! Right now the patch isn't ready for perf testing. I'm in the midst of refactoring this thing and at the moment I've got it in a broken state (doesn't build at the moment), but I promised myself I'd get some sort of update on this bug this week, so I'm posting what I have. I'll sit back down at this Tuesday and try to wrap things up then. I'll keep this bug updated.
Attachment #776741 - Attachment is obsolete: true
Alright, I'm still doing some polishing but it's time to try this out on try: https://tbpl.mozilla.org/?tree=Try&rev=4a09d8bb441c
Windows builds burned on that push, but the OS X 10.8 opt build showed an ~8.4% improvement on TART.
(In reply to Jared Wein [:jaws] from comment #37) > Windows builds burned on that push, but the OS X 10.8 opt build showed an > ~8.4% improvement on TART. This was using a comparison between UX and the try push (with an m-c base) which isn't valid. Results are here: http://compare-talos.mattn.ca/?oldRevs=d35342e7bcd0&newRev=4a09d8bb441c&submit=true
So here are results on m-c for draw performance for SVG-as-image with and without the cache: > ○ > cat without-cache-ns.log| stats > Min. 1st Qu. Median Mean 3rd Qu. Max. > 232400 488300 531100 532800 576300 892600 > > ○ > cat with-cache-ns.log| stats > Min. 1st Qu. Median Mean 3rd Qu. Max. > 17940 21700 27510 95180 31320 1241000 This is a 95% improvement for the median case. We're talking an order of magnitude here. It's pretty surprising this doesn't improve TART unless at least one of these is true: 1) SVG-as-image draw performance is not actually a limiting factor for TART. 2) TART ends up giving way too much weight to the initial draw for each image, where we draw to the cached bitmap and _then_ draw that to the screen. This is slower than just drawing the SVG directly, unsurprisingly enough (it's the source of the outliers you can see represented in the Max value for the cached case above, which is worse than Max for the uncached case). 3) There's something different on the UX branch and my measurements don't translate. I'm building a UX build right now to investigate a bit further. I've convinced myself now, though, that this is _definitely_ beneficial, so I'm going to get this patch ready for review as well.
After more investigation and discussion with Jet, it has become obvious that (3) is the case. For local testing I've been using http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/australis-liveDemo-linux.html, which I assumed represented how Australis on linux actually behaved, but that is not true! In fact, Australis does not use SVG-as-image on any platform right now. Everything that's an SVG in that demo is a PNG in the actual browser.css files used on the UX branch. So: we should not expect any perf improvement for TART from this patch, and unless Australis is going to switch to using SVG images again, this is probably misfiled as Australis:P1. However, this patch is still a _huge_ win for pages that _do_ use SVG-as-image, so I think we should check it in regardless. (Not to mention it provides infrastructure that I want to use in more situations later on.)
To assess the potential TART improvement from this caching, we could find the latest UX revision which still used SVG for drawing the tabs (let's call it rev X, where rev Y is the first which uses images instead of SVG), try to apply this patch to X, then try-push-and-TART the following: 1. X 2. Patched X 3. Y Comparing 3 to 2 would give important data when deciding if we should revert tabs drawing from images back to SVG.
(In reply to Seth Fowler [:seth] from comment #40) > After more investigation and discussion with Jet, it has become obvious that > (3) is the case. > > For local testing I've been using > http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/ > australis-liveDemo-linux.html, which I assumed represented how Australis on > linux actually behaved, but that is not true! Right, we wanted to use SVG images for the buttons on Linux but we had already switched away from SVG-as-image for the tab curve because of performance issues. We only currently use SVG for clip-paths so I filed bug 914617 once I realized caching in this bug wouldn't help that. > In fact, Australis does not use SVG-as-image on any platform right now. > Everything that's an SVG in that demo is a PNG in the actual browser.css > files used on the UX branch. > > So: we should not expect any perf improvement for TART from this patch, and > unless Australis is going to switch to using SVG images again, this is > probably misfiled as Australis:P1. However, this patch is still a _huge_ win > for pages that _do_ use SVG-as-image, so I think we should check it in > regardless. (Not to mention it provides infrastructure that I want to use in > more situations later on.) Right, and I apologize if this wasn't clear from our discussion in #developers a few weeks ago. I'm not surprised that this didn't help TART but I agree that we should take this regardless of that since it will help web content. I think Jared gave the P1 because we could take advantage of this once implemented. I don't think we're going to use SVG for linux buttons at this point so it can change priority or be removed from Australis tracking.
Is there a build with the patch I can try (for Bug 900094 but there are many other similar bugs)? I'd rather not compile this myself because I don't have any infrastructure installed, and cannot do so for quite some time. Thanks!
(In reply to Florian Bender from comment #43) > Is there a build with the patch I can try (for Bug 900094 but there are many > other similar bugs)? I'd rather not compile this myself because I don't have > any infrastructure installed, and cannot do so for quite some time. Thanks! I will push a try build shortly that you can use. (Although it will be based on the current m-c and not on Avi's build X, which I am curious about...)
Blocks: 919071
In this patch we add the cache itself. The cache is intended to be used by more things than just SVG-as-image, but for now I've avoided the extra complexity of making the key types generic. This will get handled in bug 919071.
Attachment #808054 - Flags: review?(dholbert)
This patch makes use of the surface cache introduced in the last patch to cache rasterized versions of SVGs drawn by VectorImage.
Attachment #808056 - Flags: review?(dholbert)
Attachment #798144 - Attachment is obsolete: true
Blocks: 900094
Tweaks, more comments, and (hopefully) fixes to the oranges that showed up on try.
Attachment #809413 - Flags: review?(dholbert)
Attachment #808054 - Attachment is obsolete: true
Attachment #808054 - Flags: review?(dholbert)
Comment improvements, a switch from FILTER_FAST to FILTER_NEAREST, and a rebase on top of the new part 1.
Attachment #809416 - Flags: review?(dholbert)
Attachment #808056 - Attachment is obsolete: true
Attachment #808056 - Flags: review?(dholbert)
Alright, I'm hoping that the oranges are now banished. Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=ee4889f8bef9
Oranges still seem to be there. On your next pushes, it could be useful to also request talos runs, we wanna see the wins as well ;)
Actually the most up-to-date try job is below. Solid green except for some rendering differences on Windows which I need to discuss with Daniel. https://tbpl.mozilla.org/?tree=Try&rev=9ab651cecfd4
Fixed remaining oranges other than minor rendering differences on Windows 7 and 8. The issue was that image discarding did not remove the discarded images from all data structures before. This is now fixed.
Attachment #810046 - Flags: review?(dholbert)
Attachment #809413 - Attachment is obsolete: true
Attachment #809413 - Flags: review?(dholbert)
Wonderful improvement. I used the latest try build (linux opt 32) and now there are no noticeable differences between PNG and SVG versions of testcases from bug 886279 (http://codepen.io/adrianosmond/pen/LCogn and http://philbit.com/sprites2.html). I tried at different zoom levels and SVG sprites remain sharp.
Comment on attachment 809413 [details] [diff] [review] (Part 1) - Add a temporary surface cache to imagelib. Here's a partial review of Part 1 that I wrote up yesterday [using previous version of the patch, but I'm pretty sure the comments still apply]: >diff --git a/image/build/nsImageModule.cpp b/image/build/nsImageModule.cpp [...] > imglib_Initialize() > { > mozilla::image::DiscardTracker::Initialize(); > mozilla::image::RasterImage::Initialize(); >+ mozilla::image::SurfaceCache::Initialize(); > imgLoader::GlobalInit(); > return NS_OK; > } > > static void > imglib_Shutdown() > { > imgLoader::Shutdown(); > mozilla::image::DiscardTracker::Shutdown(); >+ mozilla::image::SurfaceCache::Shutdown(); Seems like it'd be wise to invoke shutdown functions in reverse order w.r.t. their corresponding init functions. So, I'd suggest bumping SurfaceCache::Shutdown up one line, before DiscardTracker::Shutdown. >diff --git a/image/src/SurfaceCache.cpp b/image/src/SurfaceCache.cpp [...] >+#include <algorithm> >+#include "mozilla/DebugOnly.h" [...] >+ >+#include "SurfaceCache.h" #include SurfaceCache.h first, not last, in its .cpp file. This ensures that SurfaceCache.h includes or forward-declares all of the things it needs. >+struct SurfaceCost >+{ Maybe "class" instead of "struct"? Since the member-data is private, it seems like "class" would be more appropriate. >+private: >+ CachedSurface* mSurface; >+ Cost mValue; >+}; Maybe add "const" after the types here, since these values are set in the init list and then never change? >+/* >+ * XXX(seth): Right now, the key (all of the constructor parameters other than >+ * aTarget) is specialized for the needs of VectorImage; we'll generalize it in >+ * bug 919071. >+ */ [...] >+ CachedSurface(DrawTarget* aTarget, >+ Image* aImageKey, >+ const nsIntSize& aDrawableSize, >+ const size_t aDrawableBytes, >+ const gfxSize& aScale, >+ const SVGImageContext* aSVGContext, >+ float aFrame, >+ uint32_t aFlags) [...] >+ bool Matches(const nsIntSize& aDrawableSize, >+ const gfxSize& aScale, >+ const SVGImageContext* aSVGContext, >+ float aFrame, >+ uint32_t aFlags) const The comment says "the key (all of the constructor parameters other than aTarget)" -- but I don't think that's quite accurate, because Matches (which I'm assuming is matching on the multi-part key) doesn't actually take all of those constructor parameters. (It's missing aDrawableBytes and aImageKey.) (Not to mention: the constructor has aImageKey/mImageKey which sounds like *it's* reallly the key...) Maybe expand the comment a bit to explain the (various?) key(s) here? >+/* >+ * An ImageSurfaceCache is a per-image surface cache. For correctness we must be >+ * able to remove all surfaces associated with an image when the image is >+ * destroyed. Since this will happen a lot, it makes sense to make it cheap by >+ * storing the surfaces for each image separately. >+ */ Worth mentioning that lookup & removal are O(N) in the size of the cache. (Is that bad? Are we expecting this to be large?) >+ CachedSurface* Lookup(const nsIntSize& aDrawableSize, >+ const gfxSize& aScale, >+ const SVGImageContext* aSVGContext, >+ float aFrame, >+ uint32_t aFlags) >+ { >+ for (uint32_t i = 0 ; i < mSurfaces.Length() ; ++i) Remove spaces before ";" >+private: >+ nsTArray<nsRefPtr<CachedSurface>> mSurfaces; I'm pretty sure some compilers (maybe just old ones) will complain about ">>" there - add a space between the two ">" characters, or else it could be mistaken for a right-shift. >+ SurfaceCacheImpl(uint32_t aSurfaceCacheExpirationTimeMS, uint32_t aSurfaceCacheSize) >+ : mExpirationTracker(aSurfaceCacheExpirationTimeMS) >+ , mSurfaceCacheSize(aSurfaceCacheSize) >+ , mAvailableCost(0) >+ { >+ mExpirationTracker.SetSurfaceCache(this); // Avoid VC++ warning C4355. >+ } We have a preferred hackaround for C4355. :) Use "MOZ_THIS_IN_INITIALIZER_LIST()" to suppress it. (see MXR for sample usage -- basically, just substitute that macro for "this"). With that, mExpirationTracker (aka SurfaceTracker()) can then take the SurfaceCacheImpl* pointer in its constructor (and assert that it's non-null, and store it in a 'SurfaceCacheImpl* const' value since it should never be redirected to point to something else.) And then you can remove SetSurfaceCache(), since you won't need it anymore. =================== Also: a few more comments on the new things in the updated version (attachment 810046 [details] [diff] [review]), noticed when diffing prev patch vs. new patch: >+class SurfaceCacheImpl >+{ [...] >+ void Remove(CachedSurface* aSurface) >+ { >+ if (!aSurface) { >+ NS_WARNING("Removing a null surface?!"); >+ return; >+ } >+ MOZ_ASSERT(aSurface, "Should have a surface"); That early-return prevents the MOZ_ASSERT from being useful. Bump the MOZ_ASSERT up above the early-return, so that it's actually checking something. :) After that, you should drop the warning from the early-return clause (since, after you move the MOZ_ASSERT, the warning won't be printed -- any build that could print it would abort on the MOZ_ASSERT first) >+ MOZ_ASSERT(cache, "Should have an image cache for this surface"); >+ if (!cache) { >+ NS_WARNING("No image cache when removing a surface?!"); >+ return; >+ } Similar to above: drop the NS_WARNING, since we'll never reach that line in debug builds.)
Thanks for the review, Daniel! Some good comments. A few responses below. (In reply to Daniel Holbert [:dholbert] from comment #55) > #include SurfaceCache.h first, not last, in its .cpp file. This ensures that > SurfaceCache.h includes or forward-declares all of the things it needs. This is the first I've ever heard of this idea, but I have to say it makes a lot of sense! > Maybe add "const" after the types here, since these values are set in the > init list and then never change? They can be assigned to by the implicit assignment operator, unfortunately. AFAIK we can't use "= default" from C++11 right now or I'd make that more explicit. Sigh. > The comment says "the key (all of the constructor parameters other than > aTarget)" -- but I don't think that's quite accurate, because Matches (which > I'm assuming is matching on the multi-part key) doesn't actually take all of > those constructor parameters. (It's missing aDrawableBytes and aImageKey.) > > (Not to mention: the constructor has aImageKey/mImageKey which sounds like > *it's* reallly the key...) > > Maybe expand the comment a bit to explain the (various?) key(s) here? Ah, good catch! Each missing argument has a different explanation: 1. aDrawableBytes is not part of the key; it's used to determine the cost of the surface. I will refactor things a bit to clarify that. 2. aImageKey _is_ part of the key, but that part is matched already since these surfaces are stored in per-image caches. That's why it's not an argument to Match. I agree that its name sounds more "key-like" than the other parameters, but that's because eventually there will only be aImageKey and aSurfaceKey in a post bug 919071 world. > Worth mentioning that lookup & removal are O(N) in the size of the cache. > (Is that bad? Are we expecting this to be large?) I'll put an XXX. I intend to fix that in bug 919071, though I guess I have stated that nowhere. =) For most images the existing approach should be faster than a hash table since we expect only 1 surface per image (though if I was keeping it around I'd take steps to speed up bulk removal), but I still think it's preferable to switch since this isn't in a tight loop and I hate to create a performance footgun that someone could trigger if they use the wrong combination of features. > >+private: > >+ nsTArray<nsRefPtr<CachedSurface>> mSurfaces; > > I'm pretty sure some compilers (maybe just old ones) will complain about > ">>" there - add a space between the two ">" characters, or else it could be > mistaken for a right-shift. You can forever stop worrying about this, because C++11 has standardized that this parses as you'd expect. (And our compilers on all supported platforms are up-to-date enough now.) > We have a preferred hackaround for C4355. :) Use > "MOZ_THIS_IN_INITIALIZER_LIST()" to suppress it. (see MXR for sample usage > -- basically, just substitute that macro for "this"). > > With that, mExpirationTracker (aka SurfaceTracker()) can then take the > SurfaceCacheImpl* pointer in its constructor... That's how I had it before I got hit by the dreaded C4355. Thanks for letting me know about MOZ_THIS_IN_INITIALIZER_LIST! > That early-return prevents the MOZ_ASSERT from being useful. Bump the > MOZ_ASSERT up above the early-return, so that it's actually checking > something. :) Ack, whoops, I meant to remove those early returns! Those were left over from debugging.
Comment on attachment 810046 [details] [diff] [review] (Part 1) - Add a temporary surface cache to imagelib. WAVE 2 OF REVIEW COMMENTS FOR PART 1 >+class ImageSurfaceCache : public RefCounted<ImageSurfaceCache> >+{ >+public: >+ void Insert(CachedSurface* aSurface) >+ { >+ nsRefPtr<CachedSurface> surface(aSurface); >+ mSurfaces.AppendElement(aSurface); >+ } I don't think you need that local nsRefPtr. You can trust that the caller is already holding a reference to |aSurface| (otherwise, if you instantiated a nsRefPtr inside of an "if" clause here, then aSurface would die as soon as you left that if's scope, which would be pretty bad.) (In actuality, it looks like the sole caller in this patch is indeed retaining a reference, too, so you're good.) >+ CachedSurface* Lookup(const nsIntSize& aDrawableSize, >+ const gfxSize& aScale, >+ const SVGImageContext* aSVGContext, >+ float aFrame, >+ uint32_t aFlags) >+ { >+ for (uint32_t i = 0 ; i < mSurfaces.Length() ; ++i) >+ if (mSurfaces[i]->Matches(aDrawableSize, aScale, aSVGContext, aFrame, aFlags)) >+ return mSurfaces[i]; Please add curly-braces, per Moz Style Guide: "Always brace controlled statements" https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style (We let that slide sometimes for one-liner early-returns, but in a case like this with a doubly-nested statement (with one level of nesting being for a loop) we should definitely stick to the general rule.) >+ void Insert(DrawTarget* aTarget, [...] >+ nsRefPtr<CachedSurface> surface = >+ new CachedSurface(aTarget, aImageKey, aDrawableSize, aDrawableBytes, aScale, >+ aSVGContext, aFrame, aFlags); >+ SurfaceCost cost = surface->Cost(); >+ // If this is bigger than the maximum cache size, refuse to cache it. >+ if (cost.Value() > mSurfaceCacheSize) >+ return; I was *going* to say "can we move this check up a bit, before we bother allocating a CachedSurface (and use aDrawableBytes in place of cost.Value())", but then I noticed that CanHold() should take care of that. Please extend the comment here to mention that, so it's clearer that this isn't as wasteful as it might look. :) e.g.: // (Note: The caller should be checking CanHold() before inserting, // too, which should make it unlikely that we'll fail this check.) >+ // Remove elements in order of cost until we can fit this in the cache. >+ while (cost.Value() > mAvailableCost && !mCosts.IsEmpty()) >+ Remove(mCosts.LastElement().Surface()); Please add curly braces here. Unbraced loops are just asking for trouble IMHO. :) (e.g. if someone absentmindedly adds an assertion before Remove(), without noticing the missing braces, stuff's gonna break) Also: is the mCosts.IsEmpty() check required here? (Is it possible for us to break out of the loop IsEmpty() returns true, with cost.Value() still larger than mAvailableCost?) If that's possible, then this line (a few lines lower down) will be problematic: >+ mAvailableCost -= cost.Value(); Regardless, please assert that cost.Value() <= mAvailableCost before that subtraction, to be sure we can't wrap below 0 (in debug builds at least). Even with that, your "while" loop might need a bit of sanity-checking to be *sure* we aren't going to violate that. (Maybe !mCosts.IsEmpty() should be something we can assert inside the loop?) (Might even be worth adding an "OMG bail out" early-return if mAvailableCost is less than cost.Value() there (right before the subtraction) because if that fails, we'll just never stop caching stuff, because mAvailableCost will wrap to +infinity.) >+ void Remove(CachedSurface* aSurface) >+ { [...] >+ // Remove the per-image cache if it's unneeded now. >+ if (cache->IsEmpty()) >+ mImageCaches.Remove(imageKey); >+ } A curly-brace would be nice here. The juxtaposition with the de-indented "}" for the function-body makes it a bit jarring without one, IMHO. >+void ImageSurfaceCache::DiscardAll(SurfaceCacheImpl* aMainCache) >+{ >+ while (!mSurfaces.IsEmpty()) >+ aMainCache->RemoveFromCache(mSurfaces.LastElement(), this); >+} Please curly brace this as well, for reasons described above. :) >+++ b/image/src/SurfaceCache.h >+namespace mozilla { >+class SVGImageContext; >+} // namespace mozilla >+ >+namespace mozilla { >+namespace gfx { >+ class DrawTarget; >+} // namespace gfx >+} // namespace mozilla Nit: fix the inconsistent spacing before "class" between those two chunks. (Not sure which style is more common; I'm fine either way, as long as it's consistent here.)
Thanks for the comments, Daniel! A couple of responses below: (In reply to Daniel Holbert [:dholbert] from comment #57) > I was *going* to say "can we move this check up a bit, before we bother > allocating a CachedSurface (and use aDrawableBytes in place of > cost.Value())", but then I noticed that CanHold() should take care of that. CanHold() will definitely take care of that (and actually this is a sanity check, looser than CanHold()'s stricter check), but in the next version of this patch I will refactor this a bit so we can move the check up anyway. (The reason we can't right now is because CachedSurface decides on the cost, which made sense in an earlier incarnation of this code but I think no longer makes sense, so I'm going to change how things are structured a little to avoid that.) > Also: is the mCosts.IsEmpty() check required here? (Is it possible for us to > break out of the loop IsEmpty() returns true, with cost.Value() still larger > than mAvailableCost?) No, it's not, and I'll both enforce that better and add some assertions. Good catch. Again, this is leftovers from an older version of the code that behaved a bit differently.
Comment on attachment 810046 [details] [diff] [review] (Part 1) - Add a temporary surface cache to imagelib. WAVE 3 (last one) OF REVIEW COMMENTS FOR PART 1 >+++ b/image/src/SurfaceCache.cpp >+static uint32_t sSurfaceCacheExpirationTimeMS = 60 * 1000; >+static uint32_t sSurfaceCacheMaxSize = 100 * 1024 * 1024; >+static uint32_t sSurfaceCacheSizeFactor = 64; A brief comment explaining each of these values (so they're not *too* magic) would be handy. e.g.: // Default values to use if prefs are unset: // Expire cached surfaces if they're unused for 1 minute static uint32_t sSurfaceCacheExpirationTimeMS = 60 * 1000 // [next comment] <next variable> etc. >+/** >+ * SurfaceCache is an imagelib-global service that allows caching of temporary >+ * surfaces. Surfaces expire from the cache automatically if they go too long >+ * without being accessed. Optional: it's nice to have an even-shorter summary comment at the top of the file (before the #includes), too -- MXR displays this in the file-list view. See e.g. http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.h and how it affects this view here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/ >+ * @return The requested surface or nullptr if not found. >+ */ >+ static already_AddRefed<gfxDrawable> >+ Lookup(Image* aImageKey, Add comma after "The requested surface" >+ * @return False if the surface cache definitely can't hold a surface of that >+ * size. >+ */ >+ static bool CanHold(const nsIntSize& aDrawableSize); s/False/false/ >+++ b/image/src/moz.build >@@ -24,16 +24,17 @@ CPP_SOURCES += [ > 'RasterImage.cpp', >+ 'SurfaceCache.cpp', > 'SVGDocumentWrapper.cpp', > 'ScriptedNotificationObserver.cpp', > 'VectorImage.cpp', > 'imgFrame.cpp', Nit: It looks like the lexicographic ordering in this moz.build file has capital letters sorted before all lowercase letters, so you technically should be inserting SurfaceCache.cpp after "ScriptedNotificationObserver.cpp", instead of before "SVGDocumentWrapper.cpp" >diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js >+// Maximum size for the surface cache. >+pref("image.mem.surfacecache.max_size", 104857600); What are the units for this value? (Bytes, right?)Mention either in the comment or in the pref name. THOUGH -- if it's bytes -- might want to reconsider that. Looks like we have a few other prefs nearby that have a "_kb" suffix on the pref name. Seems like it'd be worth following that pattern? (using units of KB, and making the C++ code scale it appropriately) Also, this number is currently huge-and-magic-looking. Maybe add a comment explaining it (looks like it's 2^10 * 2^10 * 100, so 100 MB. It'd be worth mentioning both that expression and "100MB" things, else it just looks like a random magic number.) [general comments, without code quotes] Also: Maybe it'd be worth adding an assertion the ::Insert impl that our image has GetType() == imgIContainer::TYPE_VECTOR... (for now, just as a sanity-check) Unless that's going to change quickly enough that it's not worth it. Also: we should figure out a way to hook this up to about:memory, if you haven't already done that.
Attachment #810046 - Flags: review?(dholbert) → feedback+
Blocks: 878288
Comment on attachment 809416 [details] [diff] [review] (Part 2) - Cache rasterized surfaces in imagelib. >diff --git a/image/src/VectorImage.cpp b/image/src/VectorImage.cpp [...] >- float time = aWhichFrame == FRAME_FIRST ? 0.0f >- : mSVGDocumentWrapper->GetCurrentTime(); [...] >+ float frame = aWhichFrame == FRAME_FIRST ? 0.0f >+ : mSVGDocumentWrapper->GetCurrentTime(); I don't like "frame" as a variable-name here -- it's too overloaded of a word. (Right now, it already makes me think of nsIFrame, imgFrame, or a raster image's (integer) frame-number. I'd rather not have us name floating-point animation times "frames" too.) Maybe "timeInSVGTimeline"? or "svgDocTime"? >- AutoSVGRenderingState autoSVGState(aSVGContext, >- time, >- mSVGDocumentWrapper->GetRootSVGElem()); [...] >+ AutoSVGRenderingState autoSVGState(aSVGContext, frame, mSVGDocumentWrapper->GetRootSVGElem()); This new line is nearly 100 chars long - maybe rewrap it? >+void >+VectorImage::CreateDrawableAndShow(const SVGDrawingParameters& aParams) >+{ [...] >+ // If the image is too big to fit in the cache, don't go any further. >+ if (!SurfaceCache::CanHold(aParams.imageRect.Size())) >+ return Show(svgDrawable, aParams); "return Show()" is a bit strange, inside of a void-returning function.... Treating "void" as an actual return-type (which we can have a helper-function return on our behalf) feels a little hacky to me. (And some googling indicates that "return [expression]" is invalid in C at least, and GCC says it'll warn about it in Wreturn-type. Maybe it's allowed in C++? Even so, it's a bit unconventional & possibly worth avoiding.) Anyway -- I'd marginally prefer "Show(); [newline] return;", but if this compiles OK and you really want to keep it, I suppose it's fine. :) >+ // Try to create an offscreen surface. >+ mozilla::RefPtr<mozilla::gfx::DrawTarget> target = gfxPlatform::GetPlatform()-> >+ CreateOffscreenCanvasDrawTarget(aParams.imageRect.Size(), gfx::FORMAT_B8G8R8A8); [etc] I don't know enough about DrawTarget / gfxSurface management to feel confident r+'ing this chunk - maybe have someone graphics-ish give it a look-see? >+ // Actually draw. >+ nsRefPtr<gfxContext> ctx = new gfxContext(surface); >+ gfxUtils::DrawPixelSnapped(ctx, svgDrawable, gfxMatrix(), >+ aParams.imageRect, aParams.imageRect, >+ aParams.imageRect, aParams.imageRect, >+ gfxASurface::ImageFormatARGB32, >+ gfxPattern::FILTER_NEAREST, aParams.flags); Why are we using FILTER_NEAREST here, instead of aParams.filter? (I suppose aParams.filter will get used when we call Show()... but it seems like we might want to use the requested filter when we rasterize in the first place, too...?) If FILTER_NEAREST is better, maybe add a brief comment noting/explaining that, so it's clear it's not an oversight. >+void >+VectorImage::Show(gfxDrawable* aDrawable, const SVGDrawingParameters& aParams) >+{ >+ MOZ_ASSERT(aDrawable, "Should have a gfxDrawable by now"); >+ gfxUtils::DrawPixelSnapped(aParams.context, aDrawable, >+ aParams.userSpaceToImageSpace, >+ aParams.subimage, aParams.sourceRect, >+ aParams.imageRect, aParams.fill, >+ gfxASurface::ImageFormatARGB32, >+ aParams.filter, aParams.flags); > > MOZ_ASSERT(mRenderingObserver, "Should have a rendering observer by now"); > mRenderingObserver->ResumeHonoringInvalidations(); Hmm... Does this really belong after the "svgDrawable" draw operation? (when we draw the actual SVG) If we get an invalidation from the internal content, I'd expect us to consider that "handled" (and call ResumeHonoringInvalidations) when we've re-rasterized the SVG image, not when we've drawn an (maybe previously cached) already-rasterized surface. Speaking of which -- does the patch currently ensure that invalidations from the content will purge our cached rasterizations? If not, it should. (Maybe we need a call to SurfaceCache::Discard in SVGRootRenderingObserver::DoUpdate(), or something like that?)
Attachment #809416 - Flags: review?(dholbert) → feedback+
(In reply to Daniel Holbert [:dholbert] from comment #60) > Hmm... Does this really belong after the "svgDrawable" draw operation? > (when we draw the actual SVG) (In retrospect, that was a bit vague. To clarify: I'm wondering if we should move the ResumeHonoringInvalidations() call to a different spot, after the svgDrawable Draw operation, away from its current spot after Show()'s Draw operation)
Thanks for the review, Daniel! Some comments below: (In reply to Daniel Holbert [:dholbert] from comment #60) > "return Show()" is a bit strange, inside of a void-returning function.... > Maybe it's allowed in C++? Indeed so. =) If you're curious, see section 6.6.3 of C++11. Actually, the fact that C doesn't allow this is quite bizarre IMO. It would certainly be a mess if it wasn't allowed in C++ since it'd greatly complicate template code. > I don't know enough about DrawTarget / gfxSurface management to feel > confident r+'ing this chunk - maybe have someone graphics-ish give it a > look-see? Note that this code is nearly identical to the code currently used for caching in ClippedImage. Nevertheless, I'll get a graphics person to take a look. > Why are we using FILTER_NEAREST here, instead of aParams.filter? I'll add a comment, but to clarify: it doesn't make sense to apply an expensive filter when the image we're drawing is exactly the same size as the surface we're drawing it into. (Which is always the case here.) We do the same in VectorImage::GetFrame. > If we get an invalidation from the internal content, I'd expect us to > consider that "handled" (and call ResumeHonoringInvalidations) when we've > re-rasterized the SVG image, not when we've drawn an (maybe previously > cached) already-rasterized surface. Note that that would be a behavior change from how things work now. Right now we call ResumeHonoringInvalidations every time we Draw. Given that (as discussed below) invalidations will cause the cache to be purged, that's probably fine. However, if we were to start _not_ purging the cache for any reason (perhaps, as we discussed offline, we start caching the first frame of the animation) then this seems like it will potentially introduce bugs. We might draw, get a cache hit, not call ResumeHonoringInvalidations, and fail to notice later invalidations of the underlying document. Right now I'm inclined to continue calling ResumeHonoringInvalidations in Show. > Speaking of which -- does the patch currently ensure that invalidations from > the content will purge our cached rasterizations? If not, it should. > (Maybe we need a call to SurfaceCache::Discard in > SVGRootRenderingObserver::DoUpdate(), or something like that?) It currently does for changes caused by animation, but you made a great point on IRC that resource loading could also cause invalidations, and the result of our discussion about animations was that we probably just shouldn't try to cache them. (Since in the common case where we only have one instance of the image, we will never get a cache hit because SVG animation times don't loop, but we will still pay the cost of the caching.) We agreed that the right approach was to (1) not cache animated images in this bug, (2) purge the cache when an invalidation occurs, and (3) file a bug to cache animated images when there are multiple copies of that image visible.
Blocks: 922893
Depends on: 922899
Blocks: 923302
Attachment #809416 - Attachment is obsolete: true
Attachment #810046 - Attachment is patch: false
Attachment #810046 - Attachment is obsolete: true
Attachment #810046 - Attachment is patch: true
So I've addressed the review comments above, and moved some work that I originally planned to perform in bug 919071 into the patches for this bug. As a result, there are now more parts. =) I've removed the linear scan through the surfaces cached for a particular image that existed before, and replaced it with a hashtable lookup. This required some preliminary work. In this new part 1, I add a Hash() method to SVGImageContext and perform necessary support work. I also remove the 'const' qualifier from SVGImageContext's private member since I need it to be assignable with operator= now.
Attachment #813383 - Flags: review?(dholbert)
This new part 2 is the old part 1. The review comments above have been addressed and there has been significant refactoring. As mentioned before, the linear scan lookup in ImageSurfaceCache has been replaced with a hashtable lookup. An explicit type for keys has been defined. (Actually two, since it makes sense to separate the ImageKey from the SurfaceKey.) There are many other small improvements.
Attachment #813384 - Flags: review?(dholbert)
This is a highly simplified version of the memory reporting I had originally hoped to include. The problem is that the infrastructure isn't there to accurately measure the memory usage of the surfaces stored in the cache, and in particular to determine if they reside in CPU or GPU memory. (Bug 923928 and bug 923302 are about fixing this problem and applying the fix to SurfaceCache.) I discussed this matter with Daniel and initially did not plan to include memory reporting in this bug, but further discussion with Kyle Huey made me aware that we could report the cache size as "KIND_OTHER" memory that would not affect measurements like "heap-unclassified". This means the memory we report here may duplicated in other measurements ("heap-unclassified" or possibly one of the graphics generic texture measurements), but it still gives us something to at least look at and reason about until we add better memory reporting code in bug 923302.
Attachment #813390 - Flags: review?(dholbert)
Finally, here's part 4, which is the updated version of the previous part 2.
Attachment #813392 - Flags: review?(dholbert)
Comment on attachment 813384 [details] [diff] [review] (Part 2) - Add a temporary surface cache to imagelib. Nicholas, per Daniel's request, could you review the graphics-related code in this patch? There's really only one method that I think you need to look at here, although of course you're welcome to review as much as you want. =) Check out CachedSurface::Drawable() in SurfaceCache.cpp.
Attachment #813384 - Flags: review?(nical.bugzilla)
Comment on attachment 813392 [details] [diff] [review] (Part 4) - Cache rasterized surfaces in VectorImage. (Sorry for spelling your name wrong, Nicolas - Nicholas is how I spell _my_ name. =) So here's the other patch that has some graphics code. The key function to review here is VectorImage::CreateDrawableAndShow, which involves creating an offscreen surface and rasterizing the SVG to it.
Attachment #813392 - Flags: review?(nical.bugzilla)
Comment on attachment 813390 [details] [diff] [review] (Part 3) - Add memory reporting to the surface cache. Wow, a third Nicholas on this bug! (Counting myself.) Nicholas (Nethercote), I'm hoping you can review the memory reporting code I've added in part 3.
Attachment #813390 - Flags: review?(n.nethercote)
Attachment #813384 - Flags: review?(nical.bugzilla) → review+
Blocks: 923341
This is an updated version of part 4 which positions us to remove unnecessary Thebes contexts from CreateDrawableAndShow. (Inspired by the patches in bug 922942.) We won't be able to complete the job until bug 923338 is complete, so we'll handle this in bug 923341.
Attachment #813406 - Flags: review?(nical.bugzilla)
Attachment #813406 - Flags: review?(dholbert)
Attachment #813392 - Attachment is obsolete: true
Attachment #813392 - Flags: review?(nical.bugzilla)
Attachment #813392 - Flags: review?(dholbert)
Comment on attachment 813406 [details] [diff] [review] (Part 4) - Cache rasterized surfaces in VectorImage. Review of attachment 813406 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/VectorImage.cpp @@ +843,5 @@ > + } else { > + target = gfxPlatform::GetPlatform()-> > + CreateOffscreenCanvasDrawTarget(aParams.imageRect.Size(), gfx::FORMAT_B8G8R8A8); > + surface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(target); > + ctx = new gfxContext(surface); nit: You can move these two lines out of the if and else blocks
Attachment #813406 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 813390 [details] [diff] [review] (Part 3) - Add memory reporting to the surface cache. Review of attachment 813390 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the weak pointer removed, as described below. What kind of numbers are you getting from this reporter? ::: image/src/SurfaceCache.cpp @@ +367,5 @@ > + UNITS_BYTES, > + "Memory used by the imagelib temporary surface cache.") > + , mCache(aCache) > + { > + NS_RegisterMemoryReporter(this); We don't normally register a reporter within its constructor, but instead do it in the function that creates the reporter, e.g.: mReporter = new FooReporter(); NS_RegisterMemoryReporter(mReporter); There's nothing wrong with your way, except that it's non-typical and so might be confusing to readers. Would you mind changing it? Likewise for the unregistration. @@ +380,5 @@ > + return mCache->SizeOfSurfacesEstimate(); > + } > + > + private: > + SurfaceCacheImpl* const mCache; // Weak pointer to owner. A weak pointer is the obvious way to do this, but it's potentially dangerous. When other code iterates through the memory reporters, a strong reference to each reporter is held. In the worst case, the surface cache could be destroyed, and the reporter will be unregistered, but the reporter won't be destroyed until the iteration ends due to the strong reference from the iteration. And so Amount() might access mCache after its been freed. Now, it's very unlikely because the cache probably won't be destroyed until shutdown. But still, I try to avoid weak pointers to singleton structures in memory reporters of this kind. Can you change it to something like this: return sInstance ? sInstance->SizeOfSurfaceEstime() : 0; That's what's been done with a number of other, similar cases. I hope to fix this problem in a better way in the future (e.g. if SurfaceCacheReporter itself was a memory reporter, there wouldn't be this problem), but it's a bit tricky and still on my todo list.
Attachment #813390 - Flags: review?(n.nethercote) → review+
Thanks for the reviews, Nicolas! (In reply to Nicolas Silva [:nical] from comment #71) > nit: You can move these two lines out of the if and else blocks I'll make this change if this lands before bug 923338. I'm gonna whip up a patch for that real quick and maybe it'll be ready in time to get rid of the extra surface here.
Thanks for the review, Nicholas! (In reply to Nicholas Nethercote [:njn] from comment #72) > r=me with the weak pointer removed, as described below. I'll make those changes shortly.
This updated version of part 3 addresses Nicholas's review comments. Daniel, I haven't rerequested review from you since there's nothing in this patch other than memory reporting code. You are of course welcome to review anyway if you want. =)
Attachment #813390 - Attachment is obsolete: true
Attachment #813390 - Flags: review?(dholbert)
This needed a rebase due to changes in gfx/thebes.
Attachment #813620 - Flags: review?(dholbert)
Attachment #813406 - Attachment is obsolete: true
Attachment #813406 - Flags: review?(dholbert)
Comment on attachment 813383 [details] [diff] [review] (Part 1) - Add hashing to SVGImageContext. Sorry for the delay on this. (summit travel to Brussels, getting through bugmail/email/review backlog) :) r+ on part 1
Attachment #813383 - Flags: review?(dholbert) → review+
Comment on attachment 813384 [details] [diff] [review] (Part 2) - Add a temporary surface cache to imagelib. >+++ b/image/build/Makefile.in >@@ -6,16 +6,17 @@ > LOCAL_INCLUDES = \ > -I. \ > -I$(srcdir)/../src \ > -I$(srcdir)/../encoders/ico \ > -I$(srcdir)/../encoders/png \ > -I$(srcdir)/../encoders/jpeg \ > -I$(srcdir)/../encoders/bmp \ >+ -I$(topsrcdir)/content/svg/content/src \ > $(NULL) This is new, since the previous version of this patch. Is it necessary? Seems like it'd be nice to keep this list limited to /image subdirs (as it currently is). (If we need access to just one more header (do we?), maybe we should add it to one of the lists in http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/moz.build ?) Also: I can build with patch 1 applied, but patch 2 gives me build errors. (I tried layering #3 & #4 on top, in case those fix the issue, but they don't.) Build errors here: https://pastebin.mozilla.org/3231688
(In reply to Daniel Holbert [:dholbert] from comment #78) > This is new, since the previous version of this patch. Is it necessary? It is necessary, though I suppose we could just forward declare SurfaceCache::Init and avoid needing to include SurfaceCache.h if we made SurfaceCache and namespace instead of a class. > Seems like it'd be nice to keep this list limited to /image subdirs (as it > currently is). > > (If we need access to just one more header (do we?), maybe we should add it > to one of the lists in > http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/moz. > build ?) I'll talk to you about this on IRC as I'm not 100% sure how this is supposed to work. > Also: I can build with patch 1 applied, but patch 2 gives me build errors. Looks like to me like there are two issues here: there's been further churn in gfx/ since the last time I built the patch, and there are some incompatibilities between g++ and clang. I'll get this fixed as soon as I can.
Comment on attachment 813384 [details] [diff] [review] (Part 2) - Add a temporary surface cache to imagelib. Side question: is this hooked up to the "minimize memory usage" button in about:memory? (which says it will flush "various caches") I don't know offhand how to get that hooked up, but it seems desirable, in cases where people want Firefox to use less memory (during the minute before the expiration tracker has purged things, at least) If this isn't hooked up, please file a followup on doing that (or fix it here). Anyway - on to the code! >+++ b/image/src/SurfaceCache.cpp [...] >+#include "nsSVGEffects.h" // for SVGImageContext SVGImageContext is defined in SVGImageContext.h, not nsSVGEffects.h (Looks like we should be including that instead of nsSVGEffects.h? at which point we can drop the comment, since it'd be obvious that it's "for SVGImageContext" :)) >+#include "nsTPriorityQueue.h" This looks unused. (It's the only instance of "nsTPriorityQueue" in your patch.) Probably can be dropped? >+ typedef const T* KeyTypePointer; >+ >+ nsGenericHashKey(const T* aKey) : mKey(*aKey) { } [...] >+ bool KeyEquals(KeyTypePointer aKey) const { return *aKey == mKey; } s/const T*/KeyTypePointer/ in the constructor signature, for consistency. (That's what the first few types in nsHashKeys do, though the later ones use "const T*"... Maybe file a followup on fixing those to be consistent?) >+ static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; } >+ static PLDHashNumber HashKey(KeyTypePointer aKey) { return aKey->Hash(); } >+ enum { ALLOW_MEMMOVE = true }; >+ >+private: >+ T mKey; Reduce indentation on that last line, to align w/ code above. >+/* >+ * Cost models the cost of storing a surface in the cache. Right now, this is >+ * simply an estimate of the size of the surface, "(in bytes)" (worth being explicit about that, since the pref is in KB) >+static Cost ComputeCost(const nsIntSize aSize) >+{ >+ return aSize.width * aSize.height * 4; // width * height * 32bpp >+} The "* 32" in the comment is a bit mysterious, since the code has * 4. (bytes vs bits, but still, "* 32" is incorrect) Maybe s/32bpp/4 bytes per pixel (32bpp)/ >+ * Since we want to be able to make eviction decisions based on cost, we need to >+ * be able to look up the CachedSurface which has a certain cost as well as the >+ * cost associated with a certain CachedSurface. To make this possible, in data >+ * structures we actually store a CostEntry, which contains a weak pointer to >+ * its associated surface. Probably worth saying something about lifetimes of the CostEntry vs. the surface here, to explain why this raw pointer usage is safe. (to allay fears of people auditing this code and worrying about use-after-free on the CostEntry's raw pointer) >+class CostEntry >+{ >+public: >+ CostEntry(CachedSurface* aSurface, Cost aCost) >+ : mSurface(aSurface) >+ , mCost(aCost) >+ { } >+ >+ CachedSurface* Surface() const { return mSurface; } So the lack of a "Get" prefix means we're promising never to return null. Cool -- to enforce that, add an assertion to that effect in the constructor (and declare mSurface as "CachedSurface* const" to enforce that it doesn't change to null after-the-fact.) (Might as well declare mCost as const too, while you're at it.) >+ CachedSurface(DrawTarget* aTarget, >+ const nsIntSize aTargetSize, >+ const Cost aCost, >+ const ImageKey aImageKey, >+ const SurfaceKey& aSurfaceKey) [...] >+ { >+ MOZ_ASSERT(mImageKey, "Must have a valid image key"); >+ MOZ_ASSERT(mTarget, "Must have a valid drawable"); >+ } nit: maybe swap the order of those asserts, so they match the arg / init-list order. >+/* >+ * An ImageSurfaceCache is a per-image surface cache. For correctness we must be >+ * able to remove all surfaces associated with an image when the image is >+ * destroyed. Since this will happen a lot, it makes sense to make it cheap by >+ * storing the surfaces for each image separately. s/destroyed/destroyed or invalidated/ (right?) >+class ImageSurfaceCache : public RefCounted<ImageSurfaceCache> >+{ >+public: >+ typedef nsRefPtrHashtable<nsGenericHashKey<SurfaceKey>, CachedSurface> CacheHashtable; Maybe insert newline before "CachedSurface", to keep this under 80 chars? Also: it might improve clarity if you picked a more specific typedef-name. "CacheHashtable" is a bit generic, since you have more than one type of "cache hashtable" in this patch. (There's the other hashtable that contains ImageSurfaceCache instances -- that one doesn't happen to have a typedef, but it seems like it could equally well deserve the title of "cache hashtable".) >+ CachedSurface* Lookup(const SurfaceKey& aSurfaceKey) >+ { >+ return mSurfaces.GetWeak(aSurfaceKey); >+ } I'd rather not return a raw pointer here -- that makes ownership/lifetime unclear. Do something like this: nsRefPtr<CachedSurface> cachedSurface; mSurfaces.Get(getter_AddRefs(cachedSurface); return cachedSurface.forget(); The types increases the likelihood that we'll do our refcounting correctly, in a self-documenting way. (Note that I'm not bothering to check whether "Get()" succeeds, because it promises to null out its outparam if it fails, which is what we want.) >+/* >+ * SurfaceCacheImpl is responsible for determining which surfaces will be cached >+ * and managing the surface cache data structures. Rather than interact with >+ * SurfaceCacheImpl directly, client code interacts with SurfaceCache, which >+ * maintains high-level invariants like thread safety and encapsulates the That last bit about SurfaceCache "maintain[ing]... thread safety" is in conflict with a comment saying "SurfaceCache is not thread-safe" in the header file. Maybe by "thread safety" here you mean "ensuring it's only accessed from the main thread"? (Definitely worth clarifying that here, to avoid giving false impressions of thread safety. :)) >+class SurfaceCacheImpl >+{ >+public: >+ SurfaceCacheImpl(uint32_t aSurfaceCacheExpirationTimeMS, uint32_t aSurfaceCacheSize) >+ : mExpirationTracker(MOZ_THIS_IN_INITIALIZER_LIST(), aSurfaceCacheExpirationTimeMS) >+ , mMaxCost(aSurfaceCacheSize) Add newline before "uint32_t", and newline before "aSurfaceCacheExpirationTimeMS" to keep these lines <= 80 chars. >+ nsRefPtr<CachedSurface> surface = >+ new CachedSurface(aTarget, aTargetSize, aCost, aImageKey, aSurfaceKey); Per IRC discussion, I'm not sure if there's a reason to prefer RefPtr<> over nsRefPtr, for RefCounted<> subclasses; for now, nsRefPtr is good. (ekr just pointed me to bug 925371, whose sample code & discussion-so-far have scared me off of treating RefPtr as a drop-in nsRefPtr replacement.) >+ ImageSurfaceCache* cache = mImageCaches.GetWeak(aImageKey); >+ if (!cache) { >+ cache = new ImageSurfaceCache; >+ mImageCaches.Put(aImageKey, cache); ImageSurfaceCache derives from RefCounted<>, so "ImageSurfaceCache* cache" needs to be a nsRefPtr instead of a raw pointer, since we're sticking a newly-allocated thing into it. (And then we can "forget()" in the Put() call.) See: https://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?mark=33-36#33 >+ void Remove(CachedSurface* aSurface) >+ { >+ MOZ_ASSERT(aSurface, "Should have a surface"); >+ const ImageKey imageKey = aSurface->ImageKey(); >+ >+ ImageSurfaceCache* cache = mImageCaches.GetWeak(imageKey); This should probably be a nsRefPtr<ImageSurfaceCache> (in part because we release a reference during the run of this function, mImageCaches.Remove() call.) Right now, if someone added code after Remove() which touched the "cache" local variable, I think we'd be in trouble. >+ MOZ_ASSERT(cache, "Should have an image cache for this surface"); Assertion message is a bit vague (Who says we should have a cache for this surface?) Maybe replace with "Shouldn't be asked to remove an untracked image cache" or something like that? >+ void StartTracking(CachedSurface* aSurface) >+ { >+ CostEntry costEntry = aSurface->CostEntry(); >+ >+ mAvailableCost -= max(costEntry.Cost(), Cost(0)); The "max()" invocation there does nothing; Cost is size_t, i.e. nonnegative, so max(anyCost, 0) be anyCost by definition. I think you're trying to keep mAvailableCost from wrapping below 0, right? (Though it looks like the caller already ensures that will be the case). If you really want to cover that, you probably want something like: if (costEntry.Cost() > mAvailableCost) { MOZ_ASSERT(false, "oh noes"); mAvailableCost = 0; } else { mAvailableCost -= costEntry.Cost(); } or alternately you could just assert that costEntry.Cost() <= mAvailableCost, and depend on the client to have done the check (which it does). >+ already_AddRefed<gfxDrawable> Lookup(const ImageKey aImageKey, >+ const SurfaceKey& aSurfaceKey) >+ { >+ ImageSurfaceCache* cache = mImageCaches.GetWeak(aImageKey); >+ if (!cache) >+ return nullptr; >+ >+ CachedSurface* surface = cache->Lookup(aSurfaceKey); >+ if (!surface) >+ return nullptr; In complex code like this, it's nice to annotate early returns with a comment explaining what they mean. (Helps to clarify which early-returns are possible/unlikely/impossible-but-just-for-sanity/etc, so we can reason about when we might hit which code.) e.g. something like this for the first one: // We don't have any cached surfaces for this image. ...and for the second one: // We don't have a surface of the requested size/scale/etc. in our cache // for this image. >+ void Discard(const ImageKey aImageKey) >+ { >+ ImageSurfaceCache* cache = mImageCaches.GetWeak(aImageKey); >+ if (!cache) >+ return; As above, it'd be helpful to explain what this early-return means / how likely it is, maybe with something like this before the null-check: // Images call this method eagerly when they're destroyed, though they may // not have any cached surfaces (in which case we'll take this early return). >+ // Discard all of the images in the cache. >+ cache->ForEach(DoStopTracking, this); This is potentially O(n^2), since we're iterating across the entries (in a random order) and doing a nsTArray-removal for each one (the "RemoveElementSorted" call, which takes O(n) time because it has to shift over all of the array-entries after the removal-point). Is there any way we can avoid this hit? If not, we should at least document it here, since otherwise this ForEach() looks superficially like an O(n)-ish operation. >+ size_t mMaxCost; >+ Cost mAvailableCost; mMaxCost should be const. It's should be locked in at construction time and never change. It should also probably be of type "Cost", not "size_t", right? >+/* static */ void >+SurfaceCache::Initialize() >+{ >+ // Initialize preferences. >+ Preferences::AddUintVarCache(&sSurfaceCacheExpirationTimeMS, >+ "image.mem.surfacecache.min_expiration_ms"); >+ Preferences::AddUintVarCache(&sSurfaceCacheMaxSizeKB, >+ "image.mem.surfacecache.max_size_kb"); >+ Preferences::AddUintVarCache(&sSurfaceCacheSizeFactor, >+ "image.mem.surfacecache.size_factor"); Do we actually need these to be cached in static variables (which get updated every time the pref changes)? It looks to me like we just use them once, to initialize the SurfaceCacheImpl singleton, and then they're never touched again -- which means they could just be simplified to one-off Preferences lookups with no static variables needed. >+ // Initialize singleton SurfaceCache. >+ uint32_t surfaceSizeFactor = sSurfaceCacheSizeFactor > 0 >+ ? sSurfaceCacheSizeFactor >+ : 1; >+ uint32_t surfaceSize = PR_GetPhysicalMemorySize() / surfaceSizeFactor; >+ sInstance = new SurfaceCacheImpl(sSurfaceCacheExpirationTimeMS, >+ min(surfaceSize, sSurfaceCacheMaxSizeKB * 1024)); s/surfaceSize/surfaceCacheSize/ Also: IMHO it'd be clearer if you split out the "min()" into its own line, like this: surfaceCacheSize = min(surfaceCacheSize, sSurfaceCacheMaxSizeKB * 1024); ...so that surfaceCacheSize does actually end up being what it says on the tin. Also: Add a comment clarifying that surfaceCacheSize is in bytes (that makes the magic 1024 multiplication clearer). Also: Add a comment to assure the reader that we're not *actually allocating* that many bytes here. (For a second, I thought that was what was happening -- it superficially looks like that could be what's happening, if you read this code pessimistically :)). Also: Assert that sInstance is null before you set it. (Otherwise, it'd silently leak if we accidentally called Initialize() twice.) >+} >+ >+/* static */ void >+SurfaceCache::Shutdown() >+{ >+ delete sInstance; >+ sInstance = nullptr; Add an assertion like: MOZ_ASSERT(sInstance, "singleton is missing (maybe Shutdown was called twice?)"); at the beginning there. >+++ b/image/src/SurfaceCache.h >@@ -0,0 +1,173 @@ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+/** >+ * SurfaceCache is a service for caching temporary surfaces in imagelib. >+ */ Nice, a MXR-friendly comment! :) Please copy this to the same spot in the .cpp file, too (right after the license boilerplate), so that that file is equally friendly. >+#include "SVGImageContext.h" // for SVGImageContext >+ >+class gfxDrawable; >+ >+namespace mozilla { >+ >+class SVGImageContext; I don't think we need this forward-declare this, given that we're #including SVGImageContext. >+class SurfaceKey >+{ >+public: >+ SurfaceKey(const nsIntSize aSize, >+ const gfxSize aScale, >+ const SVGImageContext* aSVGContext, >+ const float aFrame, See beginning of comment 60 -- I'd rather not use the term "frame" to describe an SVG document time. >+ , mSVGContextIsValid(aSVGContext != nullptr) >+ , mFrame(aFrame) >+ , mFlags(aFlags) >+ { >+ if (mSVGContextIsValid) >+ mSVGContext = *aSVGContext; Might be cleaner to combine mSVGContextIsValid and mSVGContext into a single Maybe<SVGImageContext> instance. (which you can .construct() here, if given a non-null pointer; you can test it with "empty()" later on (instead of mSVGContextIsValid), and compare it for equality using .ref() if it's constructed.) It's the same amount of storage, but it gives you some debug-build guarantees that you're not touching the underlying SVGImageContext in the "not valid" case. >+ * @param aDrawableSize The dimensions of a surface in pixels. >+ * >+ * @return false if the surface cache can't hold a surface of that size. >+ */ >+ static bool CanHold(const nsIntSize& aSize); Documentation typo: s/aDrawableSize/aSize/
Attachment #813384 - Flags: review?(dholbert)
Attachment #813384 - Flags: review-
Attachment #813384 - Flags: feedback+
Thanks for the review, Daniel! (In reply to Daniel Holbert [:dholbert] from comment #80) > Might be cleaner to combine mSVGContextIsValid and mSVGContext into a single > Maybe<SVGImageContext> instance. Trust me, I wish I could do that and not enter a world of pain, but until I can get time to fix bug 913586, this is best avoided for values that need to get passed around. (As you'll note, this is based on code in ClippedImage.cpp which _does_ use Maybe<T>, but I intentionally changed it here because of Maybe<T>'s severe limitations.)
(In reply to Daniel Holbert [:dholbert] from comment #80) > Comment on attachment 813384 [details] [diff] [review] > >+ CachedSurface* Surface() const { return mSurface; } > > So the lack of a "Get" prefix means we're promising never to return null. > Cool -- to enforce that, add an assertion to that effect in the constructor > (and declare mSurface as "CachedSurface* const" to enforce that it doesn't > change to null after-the-fact.) (Might as well declare mCost as const too, > while you're at it.) Unfortunately I can't mark those fields const since this class needs to be assignable. (It has value semantics.)
(In reply to Daniel Holbert [:dholbert] from comment #80) > Comment on attachment 813384 [details] [diff] [review] > >+ typedef nsRefPtrHashtable<nsGenericHashKey<SurfaceKey>, CachedSurface> CacheHashtable; > > Maybe insert newline before "CachedSurface", to keep this under 80 chars? I tried this but it just seemed less readable to me.
(In reply to Daniel Holbert [:dholbert] from comment #80) > Comment on attachment 813384 [details] [diff] [review] > >+ mImageCaches.Put(aImageKey, cache); > > ImageSurfaceCache derives from RefCounted<>, so "ImageSurfaceCache* cache" > needs to be a nsRefPtr instead of a raw pointer, since we're sticking a > newly-allocated thing into it. (And then we can "forget()" in the Put() > call.) I don't think that calling forget() is safe in that case, because Put's second argument is of type ImageSurfaceCache* const& - how could it know that you passed it an already_AddRefed<T>? We'll just live with the refcount bouncing.
(In reply to Daniel Holbert [:dholbert] from comment #80) > Comment on attachment 813384 [details] [diff] [review] > (Part 2) - Add a temporary surface cache to imagelib. > > Side question: is this hooked up to the "minimize memory usage" button in > about:memory? (which says it will flush "various caches") I don't know > offhand how to get that hooked up, but it seems desirable, in cases where > people want Firefox to use less memory (during the minute before the > expiration tracker has purged things, at least) To do that, register an observer for the "memory-pressure" notification.
BTW, it would be awesome if this caching could apply to SVG glyphs as well as individual SVG images. I don't know whether that will readily fall out of what's already being done here, but if not, perhaps it could be a followup?
(In reply to Seth Fowler [:seth] from comment #84) > I don't think that calling forget() is safe in that case Absolutely right, sorry - I retract the "forget()" part of that statement. (But the rest stands - we should be using a nsRefPtr instead of a raw pointer there.) (In reply to Jonathan Kew (:jfkthame) from comment #86) > BTW, it would be awesome if this caching could apply to SVG glyphs > [...] perhaps it could be a followup? This bug is specific to imagelib (i.e. the code all lives in /image); bug 919084 is probably more along the lines of something that could apply to SVG glyphs.
Comment on attachment 813620 [details] [diff] [review] (Part 4) - Cache rasterized surfaces in VectorImage. >+ // Check the cache. >+ nsRefPtr<gfxDrawable> drawable = >+ SurfaceCache::Lookup(ImageKey(this), >+ SurfaceKey(params.imageRect.Size(), params.scale, Is ImageKey expected to become more complex than "Image*" at some point? If not, "ImageKey(this)" might be unnecessarily abstract; might be better to just deal with Image* directly, rather than using the charade of "wrapping it" in an ImageKey. (then we'd just pass "this" here) (The abstraction of a two-part key is somewhat compelling, but I don't think it's significantly more complex to just think in terms of there being per-Image* cache of surfaces that we're querying.) I don't feel too strongly about it, though.
Attachment #813620 - Flags: review?(dholbert) → review+
Blocks: 924807
Thanks for yet another review, Daniel! =) (In reply to Daniel Holbert [:dholbert] from comment #88) > Comment on attachment 813620 [details] [diff] [review] > Is ImageKey expected to become more complex than "Image*" at some point? I think that is likely but not certain. For subclasses of ImageResource it will probably always be just a pointer, but for ImageWrappers it may make more sense to use the address of the wrapped image combined with the parameters to the wrapper. That's because ImageWrappers can end up getting thrown away and recreated if we have to rebuild part of the layout tree, and since they currently do their own caching that trashes their cache, but it'd be nice if that didn't happen. For now I'd like to preserve the abstraction.
(In reply to Seth Fowler [:seth] from comment #89) > Thanks for yet another review, Daniel! =) No problem! :) > For now I'd like to preserve the abstraction. OK, WFM.
Addressed review comments.
Attachment #816914 - Flags: review?(dholbert)
Attachment #813384 - Attachment is obsolete: true
Rebased. Now listens for "memory-pressure" and discards everything in the cache.
Attachment #813616 - Attachment is obsolete: true
Rebased, comment improvements, rename a couple of stray |aFrame|s to |aAnimationTime|.
Attachment #813620 - Attachment is obsolete: true
Comment on attachment 816914 [details] [diff] [review] (Part 2) - Add a temporary surface cache to imagelib. >+++ b/image/build/Makefile.in >@@ -5,16 +5,17 @@ > > LOCAL_INCLUDES = \ > -I. \ > -I$(srcdir)/../src \ > -I$(srcdir)/../encoders/ico \ > -I$(srcdir)/../encoders/png \ > -I$(srcdir)/../encoders/jpeg \ > -I$(srcdir)/../encoders/bmp \ >+ -I$(topsrcdir)/content/svg/content/src \ > $(NULL) This looks out of place, inserted after the list of encoders here. I'd prefer we follow the pattern used in image/src: http://mxr.mozilla.org/mozilla-central/source/image/src/Makefile.in?mark=18-19 ...and add the new dir with "+=" and an explanatory comment along the lines of: # Because nsImageModule.cpp includes SurfaceCache.h which includes SVGImageContext.h >+++ b/image/src/SurfaceCache.cpp >+ void Discard(const ImageKey aImageKey) >+ { >+ nsRefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey); >+ if (!cache) >+ return; // No cached surfaces for this image, so nothing to do. >+ >+ // Discard all of the images in the cache. s/all of the images/all of the surfaces/ (right? We're considering just *one* image, which has (potentially) multiple surfaces.) >+ uint32_t surfaceCacheMaxSizeKB = >+ Preferences::GetUint("image.mem.surfacecache.max_size_kb", 100 * 1024); >+ >+ >+ // A knob determining the actual size of the surface cache. Currently the That double-blank-line looks unintentional - collapse to just 1 blank line? >+ uint32_t surfaceCacheSizeFactor = >+ Preferences::GetUint("image.mem.surfacecache.size_factor", 64); >+ >+ // Initialize singleton SurfaceCache. >+ uint32_t surfaceSizeFactor = surfaceCacheSizeFactor > 0 >+ ? surfaceCacheSizeFactor >+ : 1; Drop the surfaceSizeFactor local variable, and just add a clamping line after the pref-lookup -- something like: // Enforce that the factor is at least 1 (to prevent divide-by-zero below) surfaceCacheSizeFactor = max(1, surfaceCacheSizeFactor); (I suspect this superfluous local-var is just a remnant from when surfaceCacheSizeFactor was a static pref-backed variable, in the previous patch version.) >+ sInstance = new SurfaceCacheImpl(surfaceCacheExpirationTimeMS, >+ surfaceCacheSizeBytes); Per one of my notes in Comment 80, I think it'd be worth emphasizing here (with a comment) that the byte-count we're providing here is the *maximum* size of our cache - it's *not* the size of anything we're allocating right away. (When people see "new Foo(fooSizeInBytes)", you'd surely forgive them for suspecting that we might actually be allocating fooSizeInBytes of memory right then and there. We can easily quell those memory-hog-worries with an explanatory comment.) Also, this still doesn't build for me, as noted in IRC /msg. This is a enable-debug disable-optimize build, with an otherwise-empty mozconfig, with g++ 4.8.1, on 64-bit Ubuntu 13.10 prerelease. (It's unlikely that the Ubuntu prerelease is involved; if it were, I'd expect it to have given me other weird compile issues in the past, and it hasn't.) r=me with the above addressed and the build issue fixed.
Attachment #816914 - Flags: review?(dholbert) → review+
If I add #include "gfxPattern.h" to SurfaceCache.cpp, that fixes half of the build errors (the ones about incomplete type / forward declares of gfxPattern and DrawTarget). I assume you're not hitting those because of platform differences (I'll bet we have a mac-#ifdef-specific "#include gfxPattern.h" in one of the headers that gets pulled in, which saves you from this biuld error.) That just leaves the "Declaration of... changes meaning of..." build errors for Cost, ImageKey, SurfaceKey, and CostEntry. I can fix those by adding "image::" before their return types. I don't know why that's necessary, offhand, since we're already inside the "namespace image". But I can't build without it, so we need something like this. :) Here are those tweaks, in a demo patch. Please merge something along these lines into part 2.
(In reply to Daniel Holbert [:dholbert] from comment #95) > Created attachment 816989 [details] [diff] [review] > If I add #include "gfxPattern.h" to SurfaceCache.cpp, that fixes half of the > build errors Thanks for all your investigation, Daniel! I reached the same conclusion re: gfxPattern.h. This is actually bustage from bug 921753 part 2. I've posted in that bug; I imagine a fix will be forthcoming shortly. However, for now I've included gfxPattern.h temporarily. The other errors are definitely incompatibilities between g++ and clang++. As I mentioned on IRC last Friday, I'm not actually sure who's in the right here. I'm inclined just to prefix those methods with 'Get' and be done with it.
(In reply to Daniel Holbert [:dholbert] from comment #94) > Comment on attachment 816914 [details] [diff] [review] > (Part 2) - Add a temporary surface cache to imagelib. > This looks out of place, inserted after the list of encoders here. Heh, we both forgot about our previous conclusion regarding this, which is that SVGPreserveAspectRatio.h should be added to the EXPORTS group in content/svg/content/src/moz.build. I've made that change, which makes the change to image/build/moz.build unnecessary.
Cool. I assumed that maybe you'd decided against that for some reason. :) But yeah, that works too.
(In reply to Seth Fowler [:seth] from comment #96) > The other errors are definitely incompatibilities between g++ and clang++. > As I mentioned on IRC last Friday, I'm not actually sure who's in the right > here. Nor am I. :) > I'm inclined just to prefix those methods with 'Get' and be done with > it. I lean slightly towards prefixing the return type with "image::", since I don't think we should let possible-compiler-bugs dictate our method-naming schemes when there's a trivial non-naming-dependent workaround available. :) But I'm fine either way and I'm happy letting you make the call.
Fix compilation issues and review comments.
Attachment #816914 - Attachment is obsolete: true
Attachment #816915 - Attachment is obsolete: true
Here's where we stand at the moment. Looks OK on every platform except Windows. I've got a theory as to the problem and am currently trying a speculative fix. https://tbpl.mozilla.org/?tree=Try&rev=c1b39564bc04
Attachment #816918 - Attachment is obsolete: true
Comment on attachment 817034 [details] [diff] [review] (Part 2) - Add a temporary surface cache to imagelib. >+ struct SurfaceTracker : public nsExpirationTracker<CachedSurface, 2> >+ { >+ SurfaceTracker(SurfaceCacheImpl* aCache, uint32_t aSurfaceCacheExpirationTimeMS) >+ : nsExpirationTracker(aSurfaceCacheExpirationTimeMS) The errors point to this last line ^^ We probably technically need s/nsExpirationTracker/nsExpirationTracker<CachedSurface, 2>/ there, I'm guessing...
This is a bug in gcc 4.4 and below. =\ Will update the patch with a workaround.
Attachment #817034 - Attachment is obsolete: true
Whiteboard: [Australis:P1][Australis:M?] → [Australis:P1][Australis:M9]
Attachment #816989 - Attachment is obsolete: true
Blocks: 932114
Depends on: 940625
Depends on: 1003505
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: