Both classes have similar functionality. I think we can merge the GrallocPlanarYCbCrImage to GonkIOSurfaceImage class

After checking both classes on m-c, I had local patch to merge GonkIOSurfaceImage to GrallocPlanarYCbCrImage. And camera/video(sw or hw decode) work well with my changes. In my opinion, I would like to remove GonkIOSurfaceImage because it is actually a GrallocYUV Image. Let me know if you have any comment or suggestion.

(In reply to peter chang[:pchang] from comment #1) > After checking both classes on m-c, I had local patch to merge > GonkIOSurfaceImage to GrallocPlanarYCbCrImage. And camera/video(sw or hw > decode) work well with my changes. > > In my opinion, I would like to remove GonkIOSurfaceImage because it is > actually a GrallocYUV Image. Let me know if you have any comment or > suggestion. Agree!

I am doing that as part of converting GrallocYCbCrImage to the new TextureClient stuff. This a work in progress, I haven't managed to test this properly, but basically I am in the process of moving the functionalities of GonkIOSurfaceImage in GrallocPlanarYCbCrImage. also, GrallocPlanarYCbCrImage manipulates a TextureClientGralloc instead of an android GraphicBuffer directly because TextureClient can communicate properly with the TextureHost so the lifetime of the GraphicBuffer is identicall to the one of the TextureClient which let's us control who can access what properly in the compositor code. This patch also adds some IPDL stuff to allocate a GraphicBuffer without a GrallocBufferActor as propsed by Kanru in another bug, so that we get to control the lefetime of the buffer properly without the sad hacks that we do these days. Sorry this is several features in one patch because I needed to quickly put it together for the new textures, but I can separate this into several smaller patches when it works.

(In reply to Nicolas Silva [:nical] from comment #3) > Created attachment 778415 [details] [diff] [review] > Gralloc related changes to work with the new TextureClient model (WIP) > > I am doing that as part of converting GrallocYCbCrImage to the new > TextureClient stuff. > > This a work in progress, I haven't managed to test this properly, but > basically I am in the process of moving the functionalities of > GonkIOSurfaceImage in GrallocPlanarYCbCrImage. also, GrallocPlanarYCbCrImage > manipulates a TextureClientGralloc instead of an android GraphicBuffer > directly because TextureClient can communicate properly with the TextureHost > so the lifetime of the GraphicBuffer is identicall to the one of the > TextureClient which let's us control who can access what properly in the > compositor code. > > This patch also adds some IPDL stuff to allocate a GraphicBuffer without a > GrallocBufferActor as propsed by Kanru in another bug, so that we get to > control the lefetime of the buffer properly without the sad hacks that we do > these days. > > Sorry this is several features in one patch because I needed to quickly put > it together for the new textures, but I can separate this into several > smaller patches when it works. I already made the patches to remove entire GonkIOSurface changes. But your patch had higher priority to land asap. I can help you to check functionality on b2g but did you have the github or base commit number to apply your patch on b2g?

My patch is not ready to land so we can either land yours and I rebase mine on to of it, or I just pick changes from your patch and add them to mine and we land the whole thing later. I think the patch I uploaded doesn't even build yet because I am not entirely done with some parts of it. I work on top of mozilla central + the patches that are in bug 858914. Thanks a lot for proposing to help, I just started sending you emails full of questions about gralloc and other gonk stuff, and I will definitely ask for some help with testing as soon as I get the code paths that used to be GonkIOSurfaceImage to work (I am wrapping my head around GonkNativeWindow at the moment and trying to make it not use PGrallocBufferActor anymore).

(In reply to Nicolas Silva [:nical] from comment #7) > My patch is not ready to land so we can either land yours and I rebase mine > on to of it, or I just pick changes from your patch and add them to mine and > we land the whole thing later. > > I think the patch I uploaded doesn't even build yet because I am not > entirely done with some parts of it. I work on top of mozilla central + the > patches that are in bug 858914. > If so, I think I could land my patch first because I already verified the removed GrallocImage patch for camera/video. Will try to get this land these two days. > Thanks a lot for proposing to help, I just started sending you emails full > of questions about gralloc and other gonk stuff, and I will definitely ask > for some help with testing as soon as I get the code paths that used to be > GonkIOSurfaceImage to work (I am wrapping my head around GonkNativeWindow at > the moment and trying to make it not use PGrallocBufferActor anymore).

Merge GonkIOSurface to GrallocImage and remove GonkIOSurface. Now GrallocImage is still using SurfaceDescriptor directly but it will be fixed with the new textureclient/texturehost.

Comment on attachment 779550 [details] [diff] [review] merge GonkIOSurface to GrallocImage v2 Review of attachment 779550 [details] [diff] [review]: ----------------------------------------------------------------- I think we should rename GrallocPlanarYCbCrImage to something else, probably GrallocImage, since it holds not only planar ycbcr format. We could typedef PlanarYCbCrImage::Data to YCbCrData. I'm not sure if it still is a good idea for us to inherit from PlanarYCbCrImage, probably not if we will not fallback to PlanarYCbCrImage implicitly.

(In reply to Kan-Ru Chen [:kanru] from comment #11) > Comment on attachment 779550 [details] [diff] [review] > merge GonkIOSurface to GrallocImage v2 > > Review of attachment 779550 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think we should rename GrallocPlanarYCbCrImage to something else, probably > GrallocImage, since it holds not only planar ycbcr format. We could typedef So far the GrallocImage is processing the YUV data, and it could output the RGB data after converting. I would prefer to rename to GrallocYUVImage because it saves the YUV data internally. > PlanarYCbCrImage::Data to YCbCrData. I'm not sure if it still is a good idea > for us to inherit from PlanarYCbCrImage, probably not if we will not > fallback to PlanarYCbCrImage implicitly. In MediaDecoderReader, it still treats GrallocPlanarYCbCrImage as PlanarYCbCrImage. Therefore, we still need GrallocYUVImage to inherit from PlanarYCbCRImage. But it causes misunderstanding when using GrallocPlanarYCbCrImage as other YUV format, like HAL_PIXEL_FORMAT_YCrCb_420_SP. Will try to think a better way to solve it. http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderReader.cpp#227

Let's use YCbCr rather than YUV (in the compositor/Moz2D code we are renaming YUV things into YCbCr to avoid confusing people since it is the same thing). The whole Image inheritance graph is a bit quirky, we should probably reorganize it a bit later and separate interfaces from implementations. In the mean time I am not too worried about GrallocYCbCrImage inheriting from PlanarYCbCrImage since as Peter said it is used as such somewhere

Comment on attachment 779550 [details] [diff] [review] merge GonkIOSurface to GrallocImage v2 Review of attachment 779550 [details] [diff] [review]: ----------------------------------------------------------------- r=me as soon as we agree on the names.

In current Firefox OS, it is limited to ycbcr fromat. But it is not clear about the future. In android, it is not limited to ycbcr format. RGB is also used in common. For example, if we connect ANativeWindow to WebGL, RGB is used. And usage of ANativeWindow is gereralized more since android JB. We might find out more ANativeWindow's usage in Firefox OS.

What I would like is that we split Image into different interfaces Image, RGBImage, YCbCrImage, SharedImage. and implementaions like: PlanarYCbCrImage inherinting from Image and YCbCrImage GrallocImage inheriting from Image, YCbCrImage, SharedImage (and RGBImage) SharedPlanarYCbCrImage inheriting from Image, YCbCrImage, SharedImage, etc. the base Image class would have a bunch of AsYCbCrImage(), AsSharedImage(), etc. methods that would return a null pointer if the image doesn't implement the interface or if it contains data that doesn't fit the interface (like if GrallocImage is holding some YCbCr data and we call AsRGBImage()). This would simplify things a lot (for example, currentl, having SharedPlanarYCbCrImage inherit from PlanarYCbCrImage is a pain), replace all the unsafe casting all over the place by safer methods and give us the flexibility to have the gralloc image provide both the YCbCr and RGB formats. If that would work for you guys, let's do it as a followup.

(In reply to Nicolas Silva [:nical] from comment #16) > If that would work for you guys, let's do it as a followup. I find no problem about it.

If so, I will rename to GrallocImage for capability. And then create another bug to split image into different interfaces.

Rename to GrallocImage and create bug 897363 to follow up better image classes interface.

Update correct patch file....

Comment on attachment 780225 [details] [diff] [review] bug-894262-fix-v3 Review of attachment 780225 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ImageTypes.h @@ +21,2 @@ > * subtype of PlanarYCbCrImage. It takes a PlanarYCbCrImage data and can be > * used as a texture by Gonk backend directly. Say that it also takes a raw gralloc image.

Update the description in ImageType.h and add reviewers