Closed Bug 860441 Opened 7 years ago Closed 7 years ago

Camera preview update is not smooth on gonk

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: sotaro, Assigned: bjacob)

References

Details

Attachments

(1 file, 6 obsolete files)

After GFX refactoring camera preview update is not smooth.
I confirmed on unagi by using m-i code.
Summary: Camera preview update is not smooth → Camera preview update is not smooth on gonk
Assignee: nobody → bas
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.
Assignee: bas → bjacob
Attached patch WIP patch, not rendering... (obsolete) — Splinter Review
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.
Attachment #736524 - Attachment is obsolete: true
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.
Attachment #737689 - Attachment is obsolete: true
Fixed the bad texturetarget error, but still no rendering.
Attachment #737790 - Attachment is obsolete: true
(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.
Attached patch Working patch (obsolete) — Splinter Review
This patch works. Not sure if we actually -can- remove that nsIntRect like that. Would like to hear from Nical on this.
Attachment #737801 - Attachment is obsolete: true
Attachment #737891 - Flags: feedback?(nical.bugzilla)
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.
Attachment #737891 - Flags: feedback?(nical.bugzilla) → feedback+
Victory! i confirm that the latest patch works. commuting now, will clean it up before asking for review.
Attached patch working patch (obsolete) — Splinter 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.
Attachment #738068 - Flags: review?(nical.bugzilla)
Attachment #737891 - Attachment is obsolete: true
Depends on: 862425
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.
Attachment #738068 - Flags: review?(jgilbert)
(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.
Attached patch working patchSplinter Review
Attachment #738068 - Attachment is obsolete: true
Attachment #738068 - Flags: review?(nical.bugzilla)
Attachment #738068 - Flags: review?(jgilbert)
Attachment #738084 - Flags: review?(nical.bugzilla)
Attachment #738084 - Flags: review?(jgilbert)
Attachment #738084 - Flags: review?(nical.bugzilla) → review+
Blocks: 861050
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.
Attachment #738084 - Flags: review?(jgilbert)
Depends on: 864210
No longer depends on: 864210
You need to log in before you can comment on or make changes to this bug.