I noted that when ReadTexImage we stored image in big endian (http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp#391 ), and when load image from putImageData in canvas we read it as little endian in my mac(http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#3920 ).

Correctness is extremely important for this tool. As a result, I think this bug has the highest priority in all layer scope bug

Ignore my above comment. The main reason is because ReadTexImage always return BGRA format but canvas read it as RGBA format.

Comment on attachment 8381159 [details] [diff] [review] layerscope_rb_swap -> djg!

Comment on attachment 8381159 [details] [diff] [review] layerscope_rb_swap Review of attachment 8381159 [details] [diff] [review]: ----------------------------------------------------------------- This patch feels like a kludge to me. If we're inverting the sense of ENABLE_TEXTURE_RB_SWAP everytime, I think it be better to invert the logic inside ReadTexImage that picks the RG swapping mode, or ensure that the shaderConfig is correct. (One or the other seems wrong). If ReadTexImage defaults to BGRA mode, then that should be documented. ::: gfx/layers/LayerScope.cpp @@ +708,5 @@ > + // ReadTexImage below always read image as BGRA format. > + // But our layerscope tool read image buffer use canvas > + // which read image as RGBA format. So we need swap RB > + // channel here to make sure tool read correct image data. > + shaderConfig ^= ENABLE_TEXTURE_RB_SWAP; So we always flip the ENABLE_TEXTURE_RB_SWAP flag?

If we read pixels use gfxImageSurface we'll always get BGRA format. So I add a function which allow read pixels to DataSourceSurface which has more format than gfxImageSurface. Then LayerScope read pixels as RGBA format to produce correct result.

Comment on attachment 8384526 [details] [diff] [review] use_DataSourceSurface_for_ReadTexImage Review of attachment 8384526 [details] [diff] [review]: ----------------------------------------------------------------- Please please add the assert I ask for. Adding the helper function is bonus. ::: gfx/gl/GLReadTexImageHelper.cpp @@ +270,5 @@ > + uint8_t a = srcRow[3]; > + > + if (needsConvertTo16Bits) { > + destRow[0] = ((g >> 2) << 5) | ((b >> 3) << 0); > + destRow[1] = ((r >> 3) << 3) | ((g >> 5) << 0); This is more explicit as: uint16_t pixel = ((r << 11) & 0xf800) | ((g << 5) & 0x07e0) | ((b ) & 0x001f); *(uint16_t*)destRow = pixel; I would really love for this to be a static helper function like: uint16_t PackRGB565(uint8_t r, uint8_t g, uint8_t b); @@ +564,5 @@ > + destPixelSize, > + dest->Stride()); > + readSurf = dest; > + } > + MOZ_ASSERT(readAlignment); So technically readAlignment should be determined by two things: * stride * data pointer alignment Since we rely on alignment to handle non-fully-packed strides, we need to rely on our data pointer being aligned at least as well as the stride requires. Luckily, this should always be true, so we should just MOZ_ASSERT it. Add this here: MOZ_ASSERT(readSurf->GetData() % readAlignment == 0); @@ +590,5 @@ > + CopyDataSourceSurface(readSurf, dest); > + } > + > + // Check if GL is giving back 1.0 alpha for > + // RGBA reads to RGBA images from no-alpha buffers. This should probably be done immediately after ReadPixels, though it's probably fine down here.

update to address comment try push: https://tbpl.mozilla.org/?tree=Try&rev=3060234e30a7