After GFX refactoring camera preview update is not smooth. I confirmed on unagi by using m-i code.

I ran perf with b2g camera preview and got the following profiling result. It shows B2G spends lots of CPU for YUVTORGB convert. - 81.08% Camera libxul.so [.] mozilla::layers::ConvertYVU420SPToRGB565(void*, unsigned int, void*, unsigned int, void*, unsigned int, unsigned int) mozilla::layers::GonkIOSurfaceImage::GetAsSurface() ▒ mozilla::layers::ImageClientSingle::UpdateImage(mozilla::layers::ImageContainer*, unsigned int) ▒ mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*) ▒ RunnableFunction<void (*)(mozilla::layers::CompositorParent*, mozilla::layers::CompositorChild*), Tuple2<nsRefPtr<mozilla::layers::CompositorParent>, nsRefPtr<mozilla::layers::CompositorChild> > >::R▒ MessageLoop::RunTask(Task*) ▒ MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) ▒

(In reply to pchang from comment #1) > I ran perf with b2g camera preview and got the following profiling result. > > It shows B2G spends lots of CPU for YUVTORGB convert. > > - 81.08% Camera libxul.so [.] > mozilla::layers::ConvertYVU420SPToRGB565(void*, unsigned int, void*, > unsigned int, void*, unsigned int, unsigned int) > mozilla::layers::GonkIOSurfaceImage::GetAsSurface() > ▒ > > mozilla::layers::ImageClientSingle::UpdateImage(mozilla::layers:: > ImageContainer*, unsigned int) > ▒ > mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, > mozilla::layers::ImageContainer*) > ▒ > RunnableFunction<void (*)(mozilla::layers::CompositorParent*, > mozilla::layers::CompositorChild*), > Tuple2<nsRefPtr<mozilla::layers::CompositorParent>, > nsRefPtr<mozilla::layers::CompositorChild> > >::R▒ > MessageLoop::RunTask(Task*) > ▒ > MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) > ▒ Color conversion should not happen. It seems that direct bind gralloc buffer to texture rendering is not used.

Here's what I have at the moment. It's receiving buffer updates through GrallocTextureHostOGL::SwapTexturesImpl but the display is still blank black... debugging. No GL errors. Bas, you mentioned that https://bug860466.bugzilla.mozilla.org/attachment.cgi?id=736257 might help, but I don't see how?

So we looked at this again with Bas, and identified a couple of problems that were causing this to crash; this new patch is running without crashes, or GL errors, or libgenlock failures; but it still doesn't render anything.

Your turn Bas :-) this fails with invalid operation like this: 30 MOZ_CRASH(); (gdb) bt #0 0x42702e8c in mozalloc_abort (msg=<value optimized out>) at /hack/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30 #1 0x41c6f4ae in Abort (aSeverity=<value optimized out>, aStr=<value optimized out>, aExpr=<value optimized out>, aFile=<value optimized out>, aLine=1466) at /hack/mozilla-central/xpcom/base/nsDebugImpl.cpp:430 #2 NS_DebugBreak (aSeverity=<value optimized out>, aStr=<value optimized out>, aExpr=<value optimized out>, aFile=<value optimized out>, aLine=1466) at /hack/mozilla-central/xpcom/base/nsDebugImpl.cpp:387 #3 0x410dfa4c in mozilla::gl::GLContext::AfterGLCall (this=0x49590800, glFunction=0x42cb6178 "void mozilla::gl::GLContext::fEGLImageTargetTexture2D(GLenum, void*)") at ../../../dist/include/GLContext.h:1466 #4 0x41d725a8 in mozilla::gl::GLContext::fEGLImageTargetTexture2D (this=0x49590800, target=3553, image=0x48c0cc80) at ../../dist/include/GLContext.h:2854 #5 0x41d75c4c in mozilla::layers::GrallocTextureHostOGL::Lock (this=0x493b4940) at /hack/mozilla-central/gfx/layers/opengl/TextureHostOGL.cpp:698 #6 0x41d6fb8c in mozilla::layers::ImageHostSingle::Composite (this=0x4908bcc0, aEffectChain=..., aOpacity=1, aTransform=..., aOffset=..., aFilter=@0x47aff004, aClipRect=..., aVisibleRegion=0x0, aLayerProperties=0x0) at /hack/mozilla-central/gfx/layers/composite/ImageHost.cpp:76 #7 0x41d3ba80 in mozilla::layers::ImageLayerComposite::RenderLayer (this=0x49561c00, aOffset=..., aClipRect=...) at /hack/mozilla-central/gfx/layers/composite/ImageLayerComposite.cpp:90 The texture target here, mTextureTarget in the GrallocTextureHostOGL, should be TEXTURE_EXTERNAL but is TEXTURE_2D.

Fixed the bad texturetarget error, but still no rendering.

(In reply to Benoit Jacob [:bjacob] from comment #6) > Created attachment 737801 [details] > WIP patch, almost there... still no rendering > > Fixed the bad texturetarget error, but still no rendering. I'm having trouble getting my phone to run again. But I have seen video running! The key here is removing the mPictureRect = nsIntRect(0, 0, -1, -1) from ImageHostBuffered::EnsureTextureHost. This will cause your latest patch to work correctly.

This patch works. Not sure if we actually -can- remove that nsIntRect like that. Would like to hear from Nical on this.

It should be noted an alternative option if nsIntRect -shouldn't- be removed is I think to simply call UpdatePictureRect after SetDescriptor. But the order enforcement seems a little strange to me.

Comment on attachment 737891 [details] [diff] [review] Working patch Review of attachment 737891 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/ImageHost.cpp @@ -161,4 @@ > aTextureInfo); > if (result) { > mTextureHost->SetBuffer(new SurfaceDescriptor(null_t()), aAllocator); > - mPictureRect = nsIntRect(0, 0, -1, -1); D'oh! Clearing mPictureRect here on every paint was a bad thing, because we send OpUpdatePictureRect ony when the picture rect changes on the client side.

Victory! i confirm that the latest patch works. commuting now, will clean it up before asking for review.

Alright, this is ready for review. I'm asking Nical, not Bas, for review, because Bas actually wrote or suggested parts of this patch. Also, while this works stably by itself (you can stay in the camera preview for 15 minutes without any problem), there is a crash reproducible by going back and forth between the camera app and the homescreen: see bug 862425. Nical, can you please take a look at that bug while reviewing this, in case this might really be related to this patch.

Comment on attachment 738068 [details] [diff] [review] working patch Review of attachment 738068 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderEGL.cpp @@ +459,5 @@ > + mNullGraphicBuffer->getNativeBuffer(), > + attrs); > + } > + return mNullEGLImage; > + } I am not an expert on EGL and gralloc APIs, so if you have doubts about the code above you may want an extra reviewer. ::: gfx/layers/opengl/TextureHostOGL.h @@ +607,5 @@ > > + TextureSource* GetSubSource(int) MOZ_OVERRIDE > + { > + return this; > + } Why do we need this ? The texture sources that are not composed of several textures (like yuv) should return null here.

Comment on attachment 738068 [details] [diff] [review] working patch Jeff: can you please review the gralloc/EGLImage specific parts. Especially GetNullEGLImage.

(In reply to Nicolas Silva [:nical] from comment #13) > ::: gfx/layers/opengl/TextureHostOGL.h > @@ +607,5 @@ > > > > + TextureSource* GetSubSource(int) MOZ_OVERRIDE > > + { > > + return this; > > + } > > Why do we need this ? The texture sources that are not composed of several > textures (like yuv) should return null here. Without this, we crashed as the default implementation of GetSubSource was returning null, and we were then dereferencing this null pointer.

Hm. rereading what you wrote, it sounds like the code that crashed should have manually checked for null. It's in CompositorOGL.cpp, in CompositorOGL::DrawQuad: case EFFECT_YCBCR: { EffectYCbCr* effectYCbCr = static_cast<EffectYCbCr*>(aEffectChain.mPrimaryEffect.get()); TextureSource* sourceYCbCr = effectYCbCr->mTexture; const int Y = 0, Cb = 1, Cr = 2; TextureSourceOGL* sourceY = sourceYCbCr->GetSubSource(Y)->AsSourceOGL(); TextureSourceOGL* sourceCb = sourceYCbCr->GetSubSource(Cb)->AsSourceOGL(); TextureSourceOGL* sourceCr = sourceYCbCr->GetSubSource(Cr)->AsSourceOGL(); ..... BindAndDrawQuadWithTextureRect(program, effectYCbCr->mTextureCoords, sourceYCbCr->GetSubSource(Y));

... wait, we should no longer be taking this path, as we are no longer using FORMAT_YUV for these external textures. New patch coming.

(In reply to Benoit Jacob [:bjacob] from comment #15) > (In reply to Nicolas Silva [:nical] from comment #13) > > ::: gfx/layers/opengl/TextureHostOGL.h > > @@ +607,5 @@ > > > > > > + TextureSource* GetSubSource(int) MOZ_OVERRIDE > > > + { > > > + return this; > > > + } > > > > Why do we need this ? The texture sources that are not composed of several > > textures (like yuv) should return null here. > > Without this, we crashed as the default implementation of GetSubSource was > returning null, and we were then dereferencing this null pointer. Where did crash? Other TextureSource classes (except YCbCr ones) return nullptr, so it's worth looking at it.

compilation fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/307a2386c319

Comment on attachment 738084 [details] [diff] [review] working patch I guess we'll do without the 2nd review there but you're still very welcome to review post landing.

https://hg.mozilla.org/mozilla-central/rev/0d85e11dffaf https://hg.mozilla.org/mozilla-central/rev/307a2386c319