Closed Bug 975930 Opened 6 years ago Closed 6 years ago

[LayerScope] All textured buffers show RB swapped which is incorrect.

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mtseng, Assigned: mtseng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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 ).
Assignee: nobody → mtseng
Correctness is extremely important for this tool. As a result, I think this bug has the highest priority in all layer scope bug
Attached patch layerscope_rb_swap (obsolete) — Splinter Review
Ignore my above comment. The main reason is because ReadTexImage always return BGRA format but canvas read it as RGBA format.
Attachment #8381159 - Flags: review?(vladimir)
Comment on attachment 8381159 [details] [diff] [review]
layerscope_rb_swap

-> djg!
Attachment #8381159 - Flags: review?(vladimir) → review?(dglastonbury)
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?
Attachment #8381159 - Flags: review?(dglastonbury) → review-
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.
Attachment #8381159 - Attachment is obsolete: true
Attachment #8384526 - Flags: review?(jgilbert)
Attachment #8384526 - Flags: review?(dglastonbury)
Attachment #8384526 - Flags: review?(dglastonbury) → review+
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.
Attachment #8384526 - Flags: review?(jgilbert) → review+
Attachment #8388941 - Attachment is patch: true
https://hg.mozilla.org/mozilla-central/rev/bcf91c83f550
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.