830881 - LayerScope backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: LayerScope backend, v0

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 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks like it's built but pref'd off.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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);

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

All the pieces are working except gecko isn't accepting a connection on its socket.  Chasing that down ...

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 ...

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We're going(?) offline during startup and the transport service is helpfully closing our socket.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v0 with a couple of fixes

- 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%.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v0 with a couple of fixes

damn you, git.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v.-1, possibly with working RGB layers

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.
 - 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 ;).

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Hacks in color layer support

Could be done way more efficiently but I just need to see 'em for now.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Also missing: shadow canvas layer support.)

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Updated, v3

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.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Dumps shadow canvas layers with TextureImages

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?

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Additional changes/fixes

- 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.)

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 23

[Benoit Girard (:BenWa)]

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.

Comment 24

[Benoit Girard (:BenWa)]

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

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rollup patch for layerscope on b2g phone

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

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Part 0: add LZ4 to mfbt

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.

Comment 28

[Jeff Walden [:Waldo]]

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.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add support for gralloc to ReadTextureImage

This is easy to separate out.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

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...

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add LZ4 to mfbt v2

(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.

Comment 32

[Jeff Walden [:Waldo]]

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.

Comment 33

[Jeff Walden [:Waldo]]

Er, oops, didn't notice comments earlier.  Rereading those now.  :-\

Comment 34

[Jeff Walden [:Waldo]]

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.

Comment 35

[James Zhang (Spreadtrum)]

Hi all, 
  This tool is very useful for layer debugging. Do you have any schedule to push it into gecko-18/master branch?

Comment 36

[Kelsey Gilbert [:jgilbert]]

Do we still want this or is it superseded by the new layers diag stuff?

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Probably superseded; needs to be integrated (and I need to actually land the new layers diag stuff)...

Comment 38

[Till Schneidereit [:till]]

Comment on attachment 734627 [details] [diff] [review]
Add LZ4 to mfbt v2

An updated version of the integration is attached to bug 888658

Comment 39

[u459114]

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.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 41

[u459114]

(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/

Comment 42

[u480271]

I've started looking into updating this patch to work with new layers.

Comment 43

[u480271]

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 44

[u480271]

Attachment: 01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define.

Comment 45

[u480271]

Attachment: 02 - Port layerscope GL changes to new layers.

Comment 46

[u480271]

Comment on attachment 8333674 [details] [diff] [review]
01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define.

># HG changeset patch
># User Dan Glastonbury <[e-mail]>
>
>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);

Comment 47

[u480271]

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.

Comment 48

[u480271]

Comment on attachment 8333674 [details] [diff] [review]
01 - Wrap debug functions in MOZ_ENABLE_GL_TRACKING define.

Nope. This one bit rotted.

Comment 49

[u480271]

Attachment: 0001-MOZ_ENABLE_GL_TRACKING

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.

Comment 50

[u480271]

Attachment: 0002-LayerScope

Unbitrotted previous patch.

Comment 51

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=d03041c908e1

Comment 52

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 54

[u480271]

(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.

Comment 55

[u480271]

(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?

Comment 56

[u480271]

Attachment: 0002-LayerScope

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.

Comment 57

[u480271]

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 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 59

[u480271]

Removed rot and pushed to try: 
https://tbpl.mozilla.org/?tree=Try&rev=96efd741ec27

Comment 60

[u480271]

Attachment: Port layerscope GL changes to new layers.

Remove bit rot. Carry r+ :vvuk

Comment 61

[u480271]

Attachment: Wrap debug functions in MOZ_ENABLE_GL_TRACKING define.

Remove bit rot. Carry r+ :vvuk

Comment 62

[u480271]

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)

Comment 63

[u480271]

I can't obsolete the other patches. Option to obsolete doesn't appear when I select "edit patch as comment".

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2ec2143596f5
https://hg.mozilla.org/integration/b2g-inbound/rev/38c543386e0a

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ec2143596f5
https://hg.mozilla.org/mozilla-central/rev/38c543386e0a

Comment 66

[Dafeng Xu]

(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.

Comment 67

[u279076]

Does this code have or need tests?