Closed Bug 830881 Opened 12 years ago Closed 11 years ago

LayerScope backend

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: vlad, Assigned: u480271)

References

Details

Attachments

(2 files, 18 obsolete files)

49.16 KB, patch
u480271
: review+
Details | Diff | Splinter Review
4.83 KB, patch
u480271
: review+
Details | Diff | Splinter Review
Attached patch LayerScope backend, v0 (obsolete) — Splinter Review
While debugging a bug last week, I wrote a thing I'm calling LayerScope. It does real-time dumping of data from a layer manager. I did the initial implementation in the OGL layer manager, dumping relevant texture data from the various layers in the b2g compositor process. Hasn't been tested anywhere else, but should also work fine for Android. The same approach can be expanded out to other layers backends. For the implementation, it creates a raw socket where it sends some simple framed data to. You need to set up a tool called websockify (https://github.com/kanaka/websockify) to expose this as a websocket API, and then use the viewer tool (https://github.com/vvuk/layerscope -- live at http://people.mozilla.com/~vladimir/tools/layerscope/layerview.html) to view the data. (I'd love to get rid of the websockify requirement, but we don't have any simple websockets server code in the tree. Would be nice to get some!) The attached patch is the current backend. It needs some cleanup before I'll ask for review to get it checked in, specifically: - lz4 should get moved somewhere generic; I'm thinking of making it part of mfbt and giving it a simple mozilla-namespaced wrapper - The Debug.cpp file should just become LayersDebug.cpp; it doesn't really have any OGL dependencies. - The port and whether this is enabled at all should be pref-configurable, off by default.
Comment on attachment 702423 [details] [diff] [review] LayerScope backend, v0 This is crash looping for me in a debug build, on F/MOZ_Assert( 2832): Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (DebugListener not thread-safe), at /home/cjones/mozilla/new-b2g/gecko/gfx/layers/opengl/LayerManagerOGLDebug.cpp:157
Attachment #702423 - Flags: feedback-
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #0) > (I'd love to get rid of the websockify requirement, but we don't > have any simple websockets server code in the tree. Would be nice to get > some!) We added a raw TCPSocket API to gecko. It requires a permission for pages to use, and may not be built for desktop currently, but this would be a nice use case to get that ball rolling.
Looks like it's built but pref'd off.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > Comment on attachment 702423 [details] [diff] [review] > LayerScope backend, v0 > > This is crash looping for me in a debug build, on > > F/MOZ_Assert( 2832): Assertion failure: _mOwningThread.GetThread() == > PR_GetCurrentThread() (DebugListener not thread-safe), at > /home/cjones/mozilla/new-b2g/gecko/gfx/layers/opengl/LayerManagerOGLDebug. > cpp:157 -NS_IMPL_ISUPPORTS2(DebugListener, nsIRunnable, nsIServerSocketListener); +NS_IMPL_THREADSAFE_ISUPPORTS2(DebugListener, nsIRunnable, nsIServerSocketListener);
All the pieces are working except gecko isn't accepting a connection on its socket. Chasing that down ...
Bug is on the gecko side. Turning into a bit of a head-scratcher. We create the socket and bind/listen just fine. We successfully attach it to gTransportService. But something after that is closing the socket ...
We're going(?) offline during startup and the transport service is helpfully closing our socket.
Attached patch v0 with a couple of fixes (obsolete) — Splinter Review
- disables online/offline management to work around STS tearing down the server socket - fixes crasher when ReadTextureImage() fails, which seems to be happening on the magic packed gralloc buffers for some reason I'm trying to debug the video app with this, and it's exhibiting the black textures bug 100%.
Attached patch v0 with a couple of fixes (obsolete) — Splinter Review
damn you, git.
Attachment #702624 - Attachment is obsolete: true
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > Created attachment 702642 [details] [diff] [review] > v.-1, possibly with working RGB layers Or possibly not. Same symptoms as v0.
The issue isn't RGB vs. RGBA layers. I tried a patch that forces all gralloc buffers we allocate to be CONTENT_COLOR_ALPHA and get the same broken results.
Strange way this manifests 1. Open browser, load google.com. Note all layers are displayed properly. 2. Pan google.com slightly so that URL bar hides. google.com layer goes blank. 3. Pan back to top of google.com so that URL bar shows again. google.com layer comes back. We do reflow at steps (2) and (3), but the google content is the same and will be forced into RGBA buffers (because of PAINT_WILL_RESAMPLE). Also, the google.com pixels aren't being swizzled properly; blue and red are swapped. However, the homescreen layers are being swizzled correctly! Possibly a clue.
Attached patch A few more fixes (obsolete) — Splinter Review
- "fixes" blank thebes layers with voodoo: moving the DebugSendTexture() *after* the ThebesLayerOGL draw calls made it work. wtf? Should be a hint at what state we're not initializing correctly. - fixes shader for external target. Dumb typo. - adds tile iteration for "normal" (non-external) ImageLayers Still broken: reading imagelayer contents, in general. When the display only consists of imagelayers, they show up blank. If there's also a thebeslayer in there, the imagelayer rendered after it is read back successfully. Maybe another hint. Ironically, after all this, I still haven't seen the one imagelayer I wanted to with this tool ;).
Attachment #702627 - Attachment is obsolete: true
(In reply to Chris Jones [:cjones] [:warhammer] from comment #14) > Created attachment 702704 [details] [diff] [review] > A few more fixes > > - "fixes" blank thebes layers with voodoo: moving the DebugSendTexture() > *after* the ThebesLayerOGL draw calls made it work. wtf? Should be a hint > at what state we're not initializing correctly. Should add, this same voodoo unfortunately didn't seem to work for image layers.
Attached patch Hacks in color layer support (obsolete) — Splinter Review
Could be done way more efficiently but I just need to see 'em for now.
Attachment #702704 - Attachment is obsolete: true
(Also missing: shadow canvas layer support.)
Attached patch Updated, v3 (obsolete) — Splinter Review
Merges lots of stuff, and does a bunch of optimization, cleanup, and fixes. Adds color layer support, handling of null images for textures, and optimizes ReadTextureImage significantly. Also does a bunch of memory safety work.
Assignee: nobody → vladimir
Attachment #702423 - Attachment is obsolete: true
Attachment #702772 - Attachment is obsolete: true
Having some new troubles in v3 with the connection seeming to stop working. Maybe color layer support isn't pushed live to the client and it's getting confused?
Attached patch Additional changes/fixes (obsolete) — Splinter Review
- fixes R/B swizzling bugs - fixes a couple of typos in the external texture shader - removes unnecessary glFinish() - applies Vlad's fix to unbind GL_ARRAY_BUFFER which fixes unread textures - applies Y flip properly to canvas buffers With all the changes today I don't see any bugs while tracing any of the core b2g apps! \o/ (Which exercise pretty much everything.)
I have an urgent use for this tool, and was luckily able to bring it back to life locally. This would be a great project for someone to drive through! :)
Comment on attachment 702999 [details] [diff] [review] Updated, v3 Let's get some reviews going. jgilbert for the GLContext changes to ReadTextureImage; BenWa for the general layers changes/additions; jrmuizel and others for the new LayerManagerOGLDebug class. lz4 should really go elsewhere, maybe in mfbt?
Attachment #702999 - Flags: review?(jmuizelaar)
Attachment #702999 - Flags: review?(jgilbert)
Attachment #702999 - Flags: review?(bgirard)
Comment on attachment 702999 [details] [diff] [review] Updated, v3 Review of attachment 702999 [details] [diff] [review]: ----------------------------------------------------------------- This could also use a wikipage on how to use this. ::: gfx/gl/GLContext.cpp @@ +2001,5 @@ > goto cleanup; > } > > + // Ugh. This is to work around a potential bug. It shouldn't affect > + // the already bad performance much, since we'd stall in ReadPixels anyway. Can you elaborate a bit more on what that bug is? Otherwise this will stay in the code base forever since we'll never know for which bug to look for if it still exists. ::: gfx/gl/GLDefs.h @@ +3026,4 @@ > > // OES_EGL_image_external > #define LOCAL_GL_TEXTURE_EXTERNAL 0x8D65 > +#define LOCAL_GL_TEXTURE_BINDING_EXTERNAL 0x8D67 This isn't used ::: gfx/layers/Makefile.in @@ +24,5 @@ > > DEFINES += -DIMPL_THEBES > +# uncomment this to get layers logging > +# even in release builds > +DEFINES += -DFORCE_PR_LOG Should this be checked in comment then? Nice change BTW, allowing logging outside of a debug build. @@ +29,5 @@ > ifdef MOZ_DEBUG > DEFINES += -DD3D_DEBUG_INFO > endif > +# enable layerscope > +LAYERSCOPE = 1 Will this be controlled through configure.in enable-layerscope or a manual Makefile.in edit? ::: gfx/layers/opengl/LayerManagerOGL.h @@ +367,4 @@ > CreateDrawTarget(const mozilla::gfx::IntSize &aSize, > mozilla::gfx::SurfaceFormat aFormat); > > +#ifdef LAYERSCOPE LayerManagerOGL.h is exported and the value of LAYERSCOPE is only defined for files built within Graphics. This should be avoided because it's a source of headaches. ::: gfx/layers/opengl/LayerManagerOGLDebug.cpp @@ +14,5 @@ > +#include "ContainerLayerOGL.h" > +#include "ImageLayerOGL.h" > +#include "ColorLayerOGL.h" > +#include "CanvasLayerOGL.h" > +#include "TiledThebesLayerOGL.h" It's a bit sad to see this OGL specific. I hope once the layers refactor lands we'll be able to apply this kind of debugging in a way that works for all backends. @@ +63,5 @@ > +static nsCOMPtr<nsIServerSocket> gDebugServerSocket; > +static nsCOMPtr<nsIThread> gDebugSenderThread; > +static nsCOMPtr<nsISocketTransport> gDebugSenderTransport; > +static nsCOMPtr<nsIOutputStream> gDebugStream; > +static nsCOMPtr<DebugDataSender> gCurrentSender; Nit: IMO You should keep the network code separate from this file. These no value in having coupling here. @@ +421,5 @@ > + > + if (!gDebugConnected) > + return; > + > +#if 0 It's not clear why this is #if 0 ::: gfx/layers/opengl/lz4.c @@ +1,1 @@ > +/* As you said let's not land this in GFX.
Attachment #702999 - Flags: review?(bgirard) → review+
Comment on attachment 702999 [details] [diff] [review] Updated, v3 Review of attachment 702999 [details] [diff] [review]: ----------------------------------------------------------------- Opps this is r?, not f?. I'd like to see the patch again and take a closer look at LMODebug.cpp
Attachment #702999 - Flags: review+ → review-
Vlad will document this better, but a quickstart guide is 1. Install websockify [1] on your host 2. (Plug phone into USB) 3. Apply the patch here and reinstall a gecko build (./build.sh gecko && ./flash.sh gecko in the Moz system) 4. Forward layerscope socket host->phone: $ adb forward tcp:23456 tcp:23456 5. Set up layerscope websocket proxy: $ websockify localhost:23457 localhost:23456 6. Load the UI [2], connect to ws://localhost:23457 [1] https://github.com/kanaka/websockify [2] http://people.mozilla.com/~vladimir/tools/layerscope/layerview.html
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25) > 1. Install websockify [1] on your host > [1] https://github.com/kanaka/websockify On ubuntu at least, this can be a pain, but |$ sudo apt-get install python-numpy| makes it a lot easier.
Attached patch Part 0: add LZ4 to mfbt (obsolete) — Splinter Review
This adds the LZ4 functions to mfbt so that they go to a useful place and so that they can be used elsewhere. The methods are exported as static methods on a mozilla::Compression::lz4 class.
Attachment #718435 - Flags: review?(jwalden+bmo)
Comment on attachment 718435 [details] [diff] [review] Part 0: add LZ4 to mfbt Review of attachment 718435 [details] [diff] [review]: ----------------------------------------------------------------- This seems pretty reasonable to me. Style nitpicking mostly, and I'd like better types for some of the parameters than the poorly-typed underlying lz4 methods have, but it shouldn't take too much to move with this. JS already uses zlib, which we should probably add as a compression subclass too, for source compression. Given the numbers you cited on IRC (2:1 compression ratio, 20x faster than zlib), I think we seriously want to consider using lz4 for source compression instead of zlib, at least some places. 20x sounds incredibly appealing for mobile, as a tradeoff against zlib's 2.7-3:1 compression ratio. Or at least seriously worth considering. Let's file some followups to add zlib here, and to look into possibly using lz4 for source compression, possibly on a per-platform basis. ::: mfbt/Compression.cpp @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public It might be reasonable to have an LZ4.cpp that would be #include'd by Compression.cpp, so that we can separate our wrappers from the underlying code. Were any changes made to this code, to tuning parameters and the like? If yes, we should separate them out into diffs or something, so we can keep up to date with the original code. ::: mfbt/Compression.h @@ +10,5 @@ > + > +#include "mozilla/Types.h" > + > +namespace mozilla { > +namespace Compression { I think typically our namespaces have been lowercased, so mozilla::compression. @@ +12,5 @@ > + > +namespace mozilla { > +namespace Compression { > + > +class lz4 { Class naming uses InterCaps, and brace on new line. @@ +15,5 @@ > + > +class lz4 { > + > +public: > + General comments on all the methods: We should be consistent with str*/mem*/etc. functions and have destination be first in all the source/dest APIs. Per mfbt/STYLE the style for this and all the other method-overviews should be: /* * Compresses... * Destination... */ Sizes should be size_t to eliminate negative-value weird cases. If the value passed in exceeds the max size, I for size-returning methods 0 gets returned? You should explicitly clarify such cases. This'll require a little more handling code in our wrappers to handle > INT_MIN and such. But right now none of this stuff looks like it handles large ints well -- there's clear signed overflow a bunch of places, and compilers will optimize assuming that doesn't happen -- and that seems pretty dangerous. mfbt has a Range class that lightly encapsulates a pointer and a length; see Range.h. Could you pass const Range& some to make the source/destination size associations clearer? It'll make all the in/out sizes in the interfaces disappear completely, which seems great for usability. @@ +20,5 @@ > + /* > + Compresses 'isize' bytes from 'source' into 'dest'. > + Destination buffer must be already allocated, > + and must be sized to handle worst cases situations (input data not compressible) > + Worst case size evaluation is provided by function LZ4_compressBound() We should add a static method to LZ4 that exposes this: LZ4::maxCompressedSize(size_t dataSize) or something. If this is the exposed API, it'll have to expose this somehow. @@ +25,5 @@ > + > + isize : is the input size. Max supported value is ~1.9GB > + return : the number of bytes written in buffer dest > + */ > + static MFBT_API int compress(const char* source, char* dest, int isize); What do you think of making this take a dest Range, then asserting that the size of that range is big enough? Right now all the responsibility is on the caller to get the sizing right; having at least an assertion to back that up seems prudent. @@ +31,5 @@ > + /* > + Compress 'isize' bytes from 'source' into an output buffer 'dest' > + of maximum size 'maxOutputSize'. If it cannot achieve it, > + compression will stop, and result of the function will be zero. > + This function never writes outside of provided output buffer. If the source data doesn't fit, is |dest| written to? I'd assume so, and maybe you can read this to say it does, but best to make this clear. @@ +38,5 @@ > + maxOutputSize : is the size of the destination buffer (which must be already allocated) > + return : the number of bytes written in buffer 'dest' > + or 0 if the compression fails > + */ > + static MFBT_API int compress(const char* source, char* dest, int isize, int maxOutputSize); Multiple compress() overloads is going to be hard to read. Maybe tryCompress? compressInto? Not sure. @@ +46,5 @@ > + return : the number of bytes read in the source buffer > + > + If the source stream is malformed, the function will stop decoding > + and return a negative result, indicating the byte position of the > + faulty instruction A negative result that indicates the byte position? Not sure I understand what that means, exactly... @@ +54,5 @@ > + > + note : destination buffer must be already allocated. > + its size must be a minimum of 'osize' bytes. > + */ > + static MFBT_API int uncompress(const char* source, char* dest, int osize); Hmm, isn't decompress the more common name? @@ +57,5 @@ > + */ > + static MFBT_API int uncompress(const char* source, char* dest, int osize); > + > + /* > + isize : is the input size, therefore the compressed size "the length of the input compressed data" might avoid the awkwardness of size referencing here. @@ +63,5 @@ > + return : the number of bytes decoded in the destination buffer (necessarily <= maxOutputSize) > + > + If the source stream is malformed, the function will stop decoding > + and return a negative result, indicating the byte position of the > + faulty instruction. Still not sure about negative result and position. How important is it to expose the exact index? Just SIZE_MAX might be slightly easier to deal with, if the exact position isn't actually used. @@ +82,5 @@ > + isize : is the input size. Max supported value is ~1.9GB > + return : maximum output size in a "worst case" scenario > + note : this function is limited by "int" range (2^31-1) > + */ > + static MFBT_API int compressBound(int isize) { return ((isize) + ((isize)/255) + 16); } Ah, you do have the bound function -- comment earlier should refer to it. I think the name I mentioned earlier is also preferable, and clearer. Making this use size_t will avoid the int-range oddness, somewhat, but there's still overflow concern. Could you assert that no overflow happens here, at least? Or perhaps better just MOZ_CRASH() if a too-big size is passed in.
Attachment #718435 - Flags: review?(jwalden+bmo)
This is easy to separate out.
Attachment #734041 - Flags: review?(bjacob)
Comment on attachment 734041 [details] [diff] [review] Add support for gralloc to ReadTextureImage Review of attachment 734041 [details] [diff] [review]: ----------------------------------------------------------------- The plain arrays of programs/shaders business we're doing here is my only serious concern. There's a practical concern that there seems to be a bug in how many array elements we delete, see below; but beyond that, this needs to be a little bit more cleanly designed, there shouldn't be a [4] hardcoded in the header, there shouldn't be this correspondence between array indices and texture targets findable only by looking at the right spot in a bigger function in the cpp file, etc. Just a word about TEXTURE_EXTERNAL to be clear: most gralloc textures are actually plain TEXTURE_2D. TEXTURE_EXTERNAL is only going to be used for special textures, IIUC we use it only in the Camera app... still it is important to handle it. good. ::: gfx/gl/GLContext.cpp @@ +1373,5 @@ > mTexGarbageBin->GLContextTeardown(); > + > + fDeleteProgram(mReadTextureImagePrograms[0]); > + fDeleteProgram(mReadTextureImagePrograms[1]); > + fDeleteProgram(mReadTextureImagePrograms[2]); Here we stop at 2... ::: gfx/gl/GLContext.h @@ +129,5 @@ > + > + mReadTextureImagePrograms[0] = 0; > + mReadTextureImagePrograms[1] = 0; > + mReadTextureImagePrograms[2] = 0; > + mReadTextureImagePrograms[3] = 0; Here we stop at 3... consistent with the 4 below @@ +1361,4 @@ > protected: > nsDataHashtable<nsPtrHashKey<void>, void*> mUserData; > > + GLuint mReadTextureImagePrograms[4]; Here we hardcode 4...
Attachment #734041 - Flags: review?(bjacob) → review-
Attached patch Add LZ4 to mfbt v2 (obsolete) — Splinter Review
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #28) > Comment on attachment 718435 [details] [diff] [review] > Part 0: add LZ4 to mfbt > > ::: mfbt/Compression.cpp > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > It might be reasonable to have an LZ4.cpp that would be #include'd by > Compression.cpp, so that we can separate our wrappers from the underlying > code. Done. The included lz4.[ch] uses Windows new lines instead of unix ones. I'm not sure if it's better to match upstream or be consistent with the rest of the code. > > Were any changes made to this code, to tuning parameters and the like? If > yes, we should separate them out into diffs or something, so we can keep up > to date with the original code. I made a small change to fix a clang error about a cast: - size_t dec64table[] = {0, 0, 0, (size_t)-1, 0, 1, 2, 3}; + size_t dec64table[] = {0, 0, 0, -1, 0, 1, 2, 3}; This is trivial and should be upstreamed soon as part of http://code.google.com/p/lz4/issues/detail?id=64 So I'm not sure it's worth including a diff. > @@ +15,5 @@ > > + > > +class lz4 { > > + > > +public: > > + > > General comments on all the methods: > > We should be consistent with str*/mem*/etc. functions and have destination > be first in all the source/dest APIs. This is inconsistent with most compression apis (lz4, miniz, snappy) and ignoring the prior art less intuitive. i.e. you usually write A -> B (A maps to B) not B <- A (B maps from A) > > Per mfbt/STYLE the style for this and all the other method-overviews should > be: > > /* > * Compresses... > * Destination... > */ > > Sizes should be size_t to eliminate negative-value weird cases. If the > value passed in exceeds the max size, I for size-returning methods 0 gets > returned? You should explicitly clarify such cases. This'll require a > little more handling code in our wrappers to handle > INT_MIN and such. But > right now none of this stuff looks like it handles large ints well -- > there's clear signed overflow a bunch of places, and compilers will optimize > assuming that doesn't happen -- and that seems pretty dangerous. > > mfbt has a Range class that lightly encapsulates a pointer and a length; see > Range.h. Could you pass const Range& some to make the source/destination > size associations clearer? It'll make all the in/out sizes in the > interfaces disappear completely, which seems great for usability. I'm not sure I agree. The Range class is pretty unfamiliar and adds an additional cognitive barrier to users. > @@ +31,5 @@ > > + /* > > + Compress 'isize' bytes from 'source' into an output buffer 'dest' > > + of maximum size 'maxOutputSize'. If it cannot achieve it, > > + compression will stop, and result of the function will be zero. > > + This function never writes outside of provided output buffer. > > If the source data doesn't fit, is |dest| written to? I'd assume so, and > maybe you can read this to say it does, but best to make this clear. > > @@ +38,5 @@ > > + maxOutputSize : is the size of the destination buffer (which must be already allocated) > > + return : the number of bytes written in buffer 'dest' > > + or 0 if the compression fails > > + */ > > + static MFBT_API int compress(const char* source, char* dest, int isize, int maxOutputSize); > > Multiple compress() overloads is going to be hard to read. Maybe > tryCompress? compressInto? Not sure. I went with compressLimitedOutput > > @@ +46,5 @@ > > + return : the number of bytes read in the source buffer > > + > > + If the source stream is malformed, the function will stop decoding > > + and return a negative result, indicating the byte position of the > > + faulty instruction > > A negative result that indicates the byte position? Not sure I understand > what that means, exactly... I switched this to a bool that indicates failure. > > @@ +54,5 @@ > > + > > + note : destination buffer must be already allocated. > > + its size must be a minimum of 'osize' bytes. > > + */ > > + static MFBT_API int uncompress(const char* source, char* dest, int osize); > > Hmm, isn't decompress the more common name? snappy also uses uncompress. > @@ +63,5 @@ > > + return : the number of bytes decoded in the destination buffer (necessarily <= maxOutputSize) > > + > > + If the source stream is malformed, the function will stop decoding > > + and return a negative result, indicating the byte position of the > > + faulty instruction. > > Still not sure about negative result and position. How important is it to > expose the exact index? Just SIZE_MAX might be slightly easier to deal > with, if the exact position isn't actually used. As with above I return a bool and have a bool out param. > > @@ +82,5 @@ > > + isize : is the input size. Max supported value is ~1.9GB > > + return : maximum output size in a "worst case" scenario > > + note : this function is limited by "int" range (2^31-1) > > + */ > > + static MFBT_API int compressBound(int isize) { return ((isize) + ((isize)/255) + 16); } > > Ah, you do have the bound function -- comment earlier should refer to it. I > think the name I mentioned earlier is also preferable, and clearer. > > Making this use size_t will avoid the int-range oddness, somewhat, but > there's still overflow concern. Could you assert that no overflow happens > here, at least? I assert. > Or perhaps better just MOZ_CRASH() if a too-big size is > passed in.
Attachment #734627 - Flags: review?(jwalden+bmo)
Attachment #718435 - Attachment is obsolete: true
Comment on attachment 734627 [details] [diff] [review] Add LZ4 to mfbt v2 Review of attachment 734627 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay responding here. :-( This could use an update-lz4.sh script or similar, to make it trivial to pull new versions of this code. That's still useful even if we aren't going to edit the code ourselves, seems to me. I find myself somewhat leery of just asserting that size_t arguments (and values computed from them) aren't out of bounds, and kind of think having APIs that indicate fallibility would be good, but I'm not going to insist on it. (And I guess I hope that choice turns out okay in the long run, then.) What happened to using Range<char> and Range<const char> for the source/destination arguments, to more clearly associate pointers with corresponding lengths? ::: mfbt/Compression.cpp @@ +7,5 @@ > +using namespace mozilla::Compression; > + > +namespace { > + > +#include "lz4.c" Heh, beautiful. ::: mfbt/Compression.h @@ +8,5 @@ > +#ifndef mozilla_Compression_h_ > +#define mozilla_Compression_h_ > + > +#include "mozilla/Types.h" > +#include "mozilla/Assert.h" Alphabetical, and that should be "mozilla/Assertions.h". @@ +11,5 @@ > +#include "mozilla/Types.h" > +#include "mozilla/Assert.h" > + > +namespace mozilla { > +namespace Compression { "compression" for the namespace. @@ +27,5 @@ > + > +class LZ4 > +{ > + > +public: Formatting for classes is like so: class LZ4 { public: /** * Compresses 'isize' bytes from... So just indent the contents of the class by two more spaces, then. @@ +33,5 @@ > + /** > + * Compresses 'isize' bytes from 'source' into 'dest'. > + * Destination buffer must be already allocated, > + * and must be sized to handle worst cases situations (input data not compressible) > + * Worst case size evaluation is provided by function LZ4_compressBound() This should refer to maxCompressionBound(). @@ +38,5 @@ > + * > + * @param isize is the input size. Max supported value is ~1.9GB > + * @param return the number of bytes written in buffer dest > + */ > + static MFBT_API size_t compress(const char* source, size_t isize, char* dest); Having looked at src/dest algorithms recently, it seems more common for |dst| to be the first argument. Is there a reason we shouldn't make that so here, and in the others? @@ +100,5 @@ > + @return maximum output size in a "worst case" scenario > + */ > + static MFBT_API size_t maxCompressedSize(size_t isize) > + { > + size_t max = ((isize) + ((isize)/255) + 16); Method contents are two-space indented, so pull this back two.
Attachment #734627 - Flags: review?(jwalden+bmo) → review+
Er, oops, didn't notice comments earlier. Rereading those now. :-\
Okay, what you say there is reasonable enough. Although regarding Range specifically, I think that's something that, with wider use, people will get past very quickly. But it doesn't strictly need to happen now, I guess.
Hi all, This tool is very useful for layer debugging. Do you have any schedule to push it into gecko-18/master branch?
Do we still want this or is it superseded by the new layers diag stuff?
Flags: needinfo?(vladimir)
Attachment #702999 - Flags: review?(jmuizelaar)
Probably superseded; needs to be integrated (and I need to actually land the new layers diag stuff)...
Flags: needinfo?(vladimir)
Comment on attachment 734627 [details] [diff] [review] Add LZ4 to mfbt v2 An updated version of the integration is attached to bug 888658
Attachment #734627 - Attachment is obsolete: true
Hi Vlad, I am thinking about construct similar tool in bug 909192. Do you have any plan to start this work in the near future? The difference is we don't really need keep update layer architecture all the time, instead, only fetch layer architecture while user submit fetch comment from desktop. 1. Fetch layer tree and data of each layer 2. Fetch texture and relation between page.
Flags: needinfo?(vladimir)
It's actually simpler to do it this way, in theory anyway. Slower though, but you can effectively get the same behaviour by just connecting, forcing a repaint of everything, and then disconnecting to just get a single snapshot. I still do have plans on getting this cleaned up and the new layers diag stuff next week.
Flags: needinfo?(vladimir)
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #40) > It's actually simpler to do it this way, in theory anyway. Slower though, > but you can effectively get the same behaviour by just connecting, forcing a > repaint of everything, and then disconnecting to just get a single snapshot. > > I still do have plans on getting this cleaned up and the new layers diag > stuff next week. OK. Looking forward for the next step. A ref tool on another platform. http://sriramramani.wordpress.com/2013/08/26/droid-inspector/
I've started looking into updating this patch to work with new layers.
I have this working on hamachi. I suspect there a few bugs because I'm seeing 1 frame of black on images that are new. Eg. http://imgur.com/a/Y0mlM
Comment on attachment 8333674 [details] [diff] [review] 01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. ># HG changeset patch ># User Dan Glastonbury <dglastonbury@mozilla.com> > >Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. > >https://bugzilla.mozilla.org/show_bug.cgi?id=830881 > >diff --git a/gfx/gl/GLContext.cpp b/gfx/gl/GLContext.cpp >index a446938..dd35864 100644 >--- a/gfx/gl/GLContext.cpp >+++ b/gfx/gl/GLContext.cpp >@@ -3260,17 +3260,17 @@ GLContext::SetBlitFramebufferForDestTexture(GLuint aTexture) > // Note: if you are hitting this, it is likely that > // your texture is not texture complete -- that is, you > // allocated a texture name, but didn't actually define its > // size via a call to TexImage2D. > NS_RUNTIMEABORT(msg.get()); > } > } > >-#ifdef DEBUG >+#ifdef MOZ_ENABLE_GL_TRACKING > > void > GLContext::CreatedProgram(GLContext *aOrigin, GLuint aName) > { > mTrackedPrograms.AppendElement(NamedResource(aOrigin, aName)); > } > > void >diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h >index b1d6a3a..5907778 100644 >--- a/gfx/gl/GLContext.h >+++ b/gfx/gl/GLContext.h >@@ -36,16 +36,20 @@ > #include "nsAutoPtr.h" > #include "GLContextTypes.h" > #include "GLTextureImage.h" > #include "SurfaceTypes.h" > #include "GLScreenBuffer.h" > #include "GLContextSymbols.h" > #include "mozilla/GenericRefCounted.h" > >+#ifdef DEBUG >+#define MOZ_ENABLE_GL_TRACKING 1 >+#endif >+ > class nsIntRegion; > class nsIRunnable; > class nsIThread; > > namespace android { > class GraphicBuffer; > } > >@@ -576,17 +580,17 @@ private: > > // ----------------------------------------------------------------------------- > // MOZ_GL_DEBUG implementation > private: > > #undef BEFORE_GL_CALL > #undef AFTER_GL_CALL > >-#ifdef DEBUG >+#ifdef MOZ_ENABLE_GL_TRACKING > > #ifndef MOZ_FUNCTION_NAME > # ifdef __GNUC__ > # define MOZ_FUNCTION_NAME __PRETTY_FUNCTION__ > # elif defined(_MSC_VER) > # define MOZ_FUNCTION_NAME __FUNCTION__ > # else > # define MOZ_FUNCTION_NAME __func__ // defined in C99, supported in various C++ compilers. Just raw function name. >@@ -2373,24 +2377,24 @@ protected: > typedef gfx::SharedSurfaceType SharedSurfaceType; > typedef gfxImageFormat ImageFormat; > typedef gfx::SurfaceFormat SurfaceFormat; > > public: > > virtual bool MakeCurrentImpl(bool aForce = false) = 0; > >-#ifdef DEBUG >+#ifdef MOZ_ENABLE_GL_TRACKING > static void StaticInit() { > PR_NewThreadPrivateIndex(&sCurrentGLContextTLS, nullptr); > } > #endif > > bool MakeCurrent(bool aForce = false) { >-#ifdef DEBUG >+#ifdef MOZ_ENABLE_GL_TRACKING > PR_SetThreadPrivate(sCurrentGLContextTLS, this); > > // XXX this assertion is disabled because it's triggering on Mac; > // we need to figure out why and reenable it. > #if 0 > // IsOwningThreadCurrent is a bit of a misnomer; > // the "owning thread" is the creation thread, > // and the only thread that can own this. We don't >@@ -2855,17 +2859,17 @@ public: > const nsIntRegion& aDstRegion, > GLuint& aTexture, > bool aOverwrite = false, > bool aPixelBuffer = false, > GLenum aTextureUnit = LOCAL_GL_TEXTURE0, > GLenum aTextureTarget = LOCAL_GL_TEXTURE_2D); > > /** >- * Convenience wrapper around UploadImageDataToTexture for gfxASurfaces. >+ * Convenience wrapper around UploadImageDataToTexture for gfxASurfaces. > */ > SurfaceFormat UploadSurfaceToTexture(gfxASurface *aSurface, > const nsIntRegion& aDstRegion, > GLuint& aTexture, > bool aOverwrite = false, > const nsIntPoint& aSrcPoint = nsIntPoint(0, 0), > bool aPixelBuffer = false, > GLenum aTextureUnit = LOCAL_GL_TEXTURE0, >@@ -3346,17 +3350,17 @@ public: > raw_fViewport(ViewportRect().x, ViewportRect().y, > ViewportRect().width, ViewportRect().height); > } > } > > > #undef ASSERT_SYMBOL_PRESENT > >-#ifdef DEBUG >+#ifdef MOZ_ENABLE_GL_TRACKING > void CreatedProgram(GLContext *aOrigin, GLuint aName); > void CreatedShader(GLContext *aOrigin, GLuint aName); > void CreatedBuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames); > void CreatedQueries(GLContext *aOrigin, GLsizei aCount, GLuint *aNames); > void CreatedTextures(GLContext *aOrigin, GLsizei aCount, GLuint *aNames); > void CreatedFramebuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames); > void CreatedRenderbuffers(GLContext *aOrigin, GLsizei aCount, GLuint *aNames); > void DeletedProgram(GLContext *aOrigin, GLuint aName);
Attachment #8333674 - Attachment filename: Wrap-debug-functions-in-MOZENABLEGLTRACKING-define.patch → 01-Wrap-debug-functions-in-MOZENABLEGLTRACKING-define.patch
Attachment #8333676 - Attachment description: Port layerscope GL changes to new layers. → 02 - Port layerscope GL changes to new layers.
Attachment #8333676 - Attachment filename: Port-layerscope-GL-changes-to-new-layers.patch → 02-Port-layerscope-GL-changes-to-new-layers.patch
Attachment #8333674 - Attachment description: Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. → 01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define.
Comment on attachment 8333674 [details] [diff] [review] 01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. This changed looked to be independent of the rest. Someone changed all individual #ifdef DEBUG/#endif related to GL tracking to #ifdef MOZ_ENABLE_GL_TRACKING/#endif and enabled the new define unconditional under debug.
Attachment #8333674 - Flags: review?(vladimir)
Comment on attachment 8333674 [details] [diff] [review] 01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. Nope. This one bit rotted.
Attachment #8333674 - Flags: review?(vladimir)
Attached patch 0001-MOZ_ENABLE_GL_TRACKING (obsolete) — Splinter Review
This changed looked to be independent of the rest. Someone changed all individual #ifdef DEBUG/#endif related to GL tracking to #ifdef MOZ_ENABLE_GL_TRACKING/#endif and enabled the new define unconditional under debug.
Attachment #8333674 - Attachment is obsolete: true
Attachment #8334353 - Flags: review?(vladimir)
Attached patch 0002-LayerScope (obsolete) — Splinter Review
Unbitrotted previous patch.
Assignee: vladimir → dglastonbury
Attachment #8333676 - Attachment is obsolete: true
Attachment #8334439 - Flags: review?(vladimir)
Comment on attachment 8334439 [details] [diff] [review] 0002-LayerScope Review of attachment 8334439 [details] [diff] [review]: ----------------------------------------------------------------- r+ though with a few comments to fix first. Thanks for reviving this! ::: gfx/gl/GLContext.cpp @@ +1976,5 @@ > +static float* > +VertexArray() > +{ > + return gReadTextureImageVerts; > +} erm, what's the purpose of this function [looks like just to match TextureCoordArray]? Also a bare function called VertexArray() and one called TextureCoordArray() is bad, they should at least have more descriptive names... perhaps ReadTextureVertexArray and ReadTextureTexCoordArray ? @@ +2051,5 @@ > + > +GLuint > +GLContext::TextureImageProgram(int variant) > +{ > + GLuint program = mReadTextureImagePrograms[variant]; assert that variant is < the max size of programs (4, could use one of the array length macros) @@ +2099,5 @@ > + } else if (aTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_ARB) { > + variant = 3; > + } > + > + return TextureImageProgram(variant); to avoid exposing the magic variant int -- just inline the contents of this helper function here. That way the magic int isn't passed around, and it's more obvious what it's being used for (and then the inner helper doesn't have to be declared in the header, too). @@ +2162,5 @@ > + if (aTextureTarget == LOCAL_GL_TEXTURE_2D) { > + fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &oldTex); > + } else if (aTextureTarget == LOCAL_GL_TEXTURE_EXTERNAL) { > + fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_EXTERNAL, &oldTex); > + } We need to handle GL_TEXTURE_RECTANGLE here too I think. (We don't care on B2G, but we might on OSX.. and everything else supports TEX_RECT anyway.) Should probably also have an else branch that warns if an unknown target is given and returns null, e.g. if someone tries to read a 3D texture or something. @@ +2216,5 @@ > + /* Draw quad */ > + > + // this is actually red, since the resulting surface > + // has R/B in opposite order > + fClearColor(0.0f, 0.0f, 0.0f, 1.0f); That's.. actually black. This is I think the color that you get in case the draw fails. Read might be better to make it more identifiable, but see below. @@ +2219,5 @@ > + // has R/B in opposite order > + fClearColor(0.0f, 0.0f, 0.0f, 1.0f); > + fClear(LOCAL_GL_COLOR_BUFFER_BIT); > + > + fDrawArrays(LOCAL_GL_TRIANGLE_STRIP, 0, 4); You should really check the GL error before and after all of this goop; if we have an error, we should print something to the console and return null. That way we can avoid sending back just empty black frames, and have an idea of why it went wrong. @@ +2225,5 @@ > + fDisableVertexAttribArray(1); > + fDisableVertexAttribArray(0); > + > + /* Read-back draw results */ > + ReadBackPixelsIntoSurface(isurf, aSize); Same thing here about checking for an error in the readback. ::: gfx/gl/GLContext.h @@ +2797,2 @@ > const gfxIntSize& aSize, > + int aShaderProgram, This shouldn't take an int; it should really take ShaderProgramType. Is the #include goop too convoluted to get i in here? (it might be -- in which case maybe just some doc comments) ::: gfx/layers/LayerScope.cpp @@ +481,5 @@ > + aSource->BindTexture(LOCAL_GL_TEXTURE0); > + > + GLuint textureId = 0; > + // This is horrid hack. It assumes that aGLContext matches the context > + // aSource has bound to. Hm, I think this assumption is safe/correct; is there any way for us to assert? (Either matches, or is in a share group with) @@ +486,5 @@ > + if (textureTarget == LOCAL_GL_TEXTURE_2D) { > + aGLContext->GetUIntegerv(LOCAL_GL_TEXTURE_BINDING_2D, &textureId); > + } else if (textureTarget == LOCAL_GL_TEXTURE_EXTERNAL) { > + aGLContext->GetUIntegerv(LOCAL_GL_TEXTURE_BINDING_EXTERNAL, &textureId); > + } May as well handle GL_TEXTURE_RECTANGLE here too. ::: gfx/layers/opengl/CompositorOGL.cpp @@ +762,5 @@ > { > PROFILER_LABEL("CompositorOGL", "BeginFrame"); > MOZ_ASSERT(!mFrameInProgress, "frame still in progress (should have called EndFrame or AbortFrame"); > > + LayerScope::BeginFrame(mGLContext, PR_Now()); We *might* already have an appropriate frame time stored away somewhere. Take a look to see if you can find it to avoid calling PR_Now again.
Attachment #8334439 - Flags: review?(vladimir)
Attachment #8334439 - Flags: review?(jmuizelaar)
Attachment #8334439 - Flags: review+
Comment on attachment 8334353 [details] [diff] [review] 0001-MOZ_ENABLE_GL_TRACKING Review of attachment 8334353 [details] [diff] [review]: ----------------------------------------------------------------- I actually don't even think this is needed any more; originally I wanted to dump all textures etc., but that ended up not being possible because I needed info about each texture (size/format) since that can't be queried on ES. So we no longer use any of the mTracked* bits in layerscope, thus I'm pretty sure this doesn't need to change.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #53) > Comment on attachment 8334353 [details] [diff] [review] > 0001-MOZ_ENABLE_GL_TRACKING > > Review of attachment 8334353 [details] [diff] [review]: > ----------------------------------------------------------------- > > I actually don't even think this is needed any more; originally I wanted to > dump all textures etc., but that ended up not being possible because I > needed info about each texture (size/format) since that can't be queried on > ES. So we no longer use any of the mTracked* bits in layerscope, thus I'm > pretty sure this doesn't need to change. The patch just wraps up what was many #ifdef _DEBUG/#endif into one switch, which makes it easy to enable/disable the feature. Or do you mean that the CreatedXXX functions can be removed? Appears to be used in GLContext destructor to report GL objects that haven't been released.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #52) > Comment on attachment 8334439 [details] [diff] [review] > 0002-LayerScope > > Review of attachment 8334439 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ though with a few comments to fix first. Thanks for reviving this! > > ::: gfx/gl/GLContext.cpp > @@ +1976,5 @@ > > +static float* > > +VertexArray() > > +{ > > + return gReadTextureImageVerts; > > +} > > erm, what's the purpose of this function [looks like just to match > TextureCoordArray]? Yes --- for the symmetry with TextureCoordArray. > perhaps ReadTextureVertexArray and ReadTextureTexCoordArray ? Sounds good. > @@ +2216,5 @@ > > + /* Draw quad */ > > + > > + // this is actually red, since the resulting surface > > + // has R/B in opposite order > > + fClearColor(0.0f, 0.0f, 0.0f, 1.0f); > > That's.. actually black. This is I think the color that you get in case the > draw fails. Read might be better to make it more identifiable, but see > below. The comment stands from the previous patch. I looked at the comment and thought "but that's opaque black". > ::: gfx/gl/GLContext.h > @@ +2797,2 @@ > > const gfxIntSize& aSize, > > + int aShaderProgram, > > This shouldn't take an int; it should really take ShaderProgramType. Is the > #include goop too convoluted to get i in here? (it might be -- in which case > maybe just some doc comments) The reason I pass aShaderProgram as int is because I didn't want to suck LayerManagerOGLProgram.h into GLContext.h, which is a central header. MSVC lets for forward declare enums, but GCC/Clang aren't happy about that. > > ::: gfx/layers/LayerScope.cpp > @@ +481,5 @@ > > + aSource->BindTexture(LOCAL_GL_TEXTURE0); > > + > > + GLuint textureId = 0; > > + // This is horrid hack. It assumes that aGLContext matches the context > > + // aSource has bound to. > > Hm, I think this assumption is safe/correct; is there any way for us to > assert? (Either matches, or is in a share group with) SourceOGL exposes most of the texture data except for textureId. I've changed the behaviour of ReadTextureImage to assume the texture to copy is bound to GL_TEXTURE0 before it's called. There could quite possibly be a better way?
Attached patch 0002-LayerScope (obsolete) — Splinter Review
Updated with review comments from :vlad and fixed bug in shader program creation that resulted in no program being returned on the first use of a variant. Carrying the r? for Jeff from the previous version of patch.
Attachment #8334439 - Attachment is obsolete: true
Attachment #8334439 - Flags: review?(jmuizelaar)
Attachment #8335773 - Flags: review?(vladimir)
Attachment #8335773 - Flags: review?(jmuizelaar)
Flags: needinfo?(vladimir)
To get this to work on b2g: $ ./edit-prefs.sh Add: user_pref("gfx.layerscope.enable", true); user_pref("network.gonk.manage-offline-status", false);
Comment on attachment 8335773 [details] [diff] [review] 0002-LayerScope Looks good to me, tryserver and then land away (via checkin-needed).. Jeff can look at it when he's back
Attachment #8335773 - Flags: review?(vladimir) → review+
Flags: needinfo?(vladimir)
Attachment #8334353 - Flags: review?(vladimir) → review+
Remove bit rot. Carry r+ :vvuk
Attachment #8335773 - Attachment is obsolete: true
Attachment #8335773 - Flags: review?(jmuizelaar)
Remove bit rot. Carry r+ :vvuk
Attachment #8334353 - Attachment is obsolete: true
Attachment #8340205 - Flags: review+
Attachment #8340208 - Flags: review+
Please check in: Wrap debug functions in MOZ_ENABLE_GL_TRACKING define. (4.83 KB, patch) Port layerscope GL changes to new layers. (49.16 KB, patch)
Keywords: checkin-needed
Attachment #702999 - Attachment is obsolete: true
Attachment #702999 - Flags: review?(jgilbert)
I can't obsolete the other patches. Option to obsolete doesn't appear when I select "edit patch as comment".
Attachment #703131 - Attachment is obsolete: true
Attachment #734041 - Attachment is obsolete: true
Attachment #717010 - Attachment is obsolete: true
Attachment #703173 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #65) > https://hg.mozilla.org/mozilla-central/rev/2ec2143596f5 > https://hg.mozilla.org/mozilla-central/rev/38c543386e0a Hi Ryan, Can you tell me what means the version 'mozilla28', I apply the patch in v1.2 and v1.3 and failed.
Blocks: LayerScope
Does this code have or need tests?
Flags: in-testsuite?
Blocks: 991227
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: