897515 - LayerView sets a RGB_565 pixel format on the surface holder even when we're drawing in 24-bit

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I don't know what effect, if any, this has, but we should probably take this out if it doesn't break anything. Specially since now we are rending in 24-bit in some cases, and telling the SurfaceHolder that we're rendering in 565 might have unexpected consequences.

Some archaeology shows this has been present since the original version of FlexibleGLSurfaceView that patrick walton checked in at the start of new fennec.

http://hg.mozilla.org/mozilla-central/file/b3fcd828cadc/mobile/android/base/gfx/LayerView.java#l281

For anybody who wants to take on this bug, it's basically removing that line of code and checking to see if it breaks anything. It should be tested on a device where we render in 16-bit and a device where we render in 24-bit (as of bug 803299).

Comment 1

[Alexandru Chiriac]

Hello, I am new to Firefox mobile development and I would like to work on this bug as a starter. I've managed to succesfully build and run Fennec.

Thanks, 
Alex

Comment 2

[Kartikaya Gupta (email:[e-mail])]

That's awesome, thanks! I'm assigning the bug to you. To fix this bug, you basically need to remove the line of code indicated in comment 0, and then test it in 16-bit and 24-bit modes. By default Fennec will run on your device in one of these modes, depending on the return value of GeckoAppShell.getScreenDepth(). You can temporarily change the return value of getScreenDepth() to also exercise the other mode. Once you have verified that both modes work ok (any graphical problems should be fairly obvious if you load a few pages and navigate around), then you can put together a patch for review. There are instructions on how to do this at [1]. Please don't hesitate to ask questions if you have any, either in this bug, on IRC, or by email.

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Comment 3

[Alexandru Chiriac]

Attachment: Removed the RGB_565 pixel format LayerView setting on the surface holder. Also removed PixelFormat import (unused after removing the format setting).

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8404022 [details] [diff] [review]
Removed the RGB_565 pixel format LayerView setting on the surface holder. Also removed PixelFormat import (unused after removing the format setting).

Review of attachment 8404022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Can you confirm that you verified Fennec works fine in both 16-bit and 24-bit mode?

Also, the commit message should be updated so that it reflects what the patch is intending to accomplish; right now it describes what the patch does which is evident to anybody who looks at the patch contents. For example, you could use a commit message like "Bug 897515 - Remove unneeded pixel format set on the LayerView surface holder. r=kats" The use of the word "unneeded" in this version indicates that we are removing code that is no longer needed. The r=kats at the end indicates that I have reviewed it.

Please upload an updated patch with a new commit message, and we can get it landed. Thanks!

Comment 5

[Alexandru Chiriac]

Attachment: Remove unneeded pixel format set on the LayerView surface holder.

Comment 6

[Alexandru Chiriac]

I've tested Fennec in both 16-bit and 24-bit modes and it worked fine.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Great work, thanks!

I'm marking the bug checkin-needed so that the sheriffs will come along and land this patch. You're welcome to take a look at other mentored bugs and pick one up to work on.

Comment 8

[Carsten Book [:Tomcat]]

landed on fx-team. Great work, welcome! and thanks for contributing to Mozilla!

https://hg.mozilla.org/integration/fx-team/rev/b962eae80edf

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b962eae80edf