828289 - - Crash when sending camera app to foreground

Status: RESOLVED DUPLICATE of bug 826829

(Core :: DOM: Device Interfaces, defect)

Description

[Sotaro Ikeda [:sotaro]]

This bug is created as part of Bug 826829. This handles a crash when changing camera app to foreground. Bug 826829 handles a cash when sending camera app to background. 

The story to reproduce the crash is same as Bug 826829. It happens when I repeat [1][2] many times.

[1]. start Camera application. Wait until a camera preview becomes active. 
[2]. push home button

Comment 1

[Sotaro Ikeda [:sotaro]]

following is copied from Bug 826829.

[2] abort on re-starting preview
camera app stop preview correctly and try to start preview again. 
There is a Image(GonkIOSurfaceImage) from last preview. And the Image references to a CameraGraphicBuffer.
When prevew starts, ImageContainer::SetCurrentImage() is called.
And within it, ImageContainerChild::SendImageAsync() is called and mActiveImage is replaced to new Image.
When mActiveImage is replaced, there is no refence to the GonkIOSurfaceImage from previous preview and the the GonkIOSurfaceImage is deleted.
During the destruction of the GonkIOSurfaceImage, CameraGraphicBuffer::Unlock() is called and Gralloc buffer is deleted.
After that on parent side, ImageContainerParent::RecvPublishImage() is called and send back previous image. But it is already deallocated on child side.

Comment 2

[Sotaro Ikeda [:sotaro]]

following explains about the cause of the bug

- ImageContainer::SetCurrentImage() keeps a reference only to a current active image
- when rendering via ImageContainerChild, it is necessary that previous gralloc buffer neededs to be alive.
  becuase previous gralloc buffer will be sent back to ImageContainerChild during next image rendering.
- In normal situation, rendering gralloc buffer is kept alive by GonkNativeWindow
- when camera app go back to background, GonkNativeWindow do not keep rendering gralloc buffer as alive
- when starting preview again, ImageContainer::SetCurrentImage() destroy preview image(previous gralloc buffer)
- ImageContainerChild might receive previous gralloc buffer after delete it

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: Patch: ImageContainer keeps a previsous Image when rendering via ImageContainerChild

By the patch, ImageContainer prevents previsous Image from de-allocated when rendering via ImageContainerChild.

Comment 4

[Andrew Overholt [:overholt]]

Do we need to find a reviewer here?

Comment 5

[Lucas Adamski [:ladamski]]

Blocking for 1.0 not for basecamp.

Comment 6

[Sotaro Ikeda [:sotaro]]

I am going to ask Kan-Ru Chen to review.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

We need this to be fixed in order to fix bug 826829.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: H.264 video crash log after applying attachment 699791

attachment 699791 [details] [diff] [review] is going to keep gralloc buffer in ImageContainer. When applying it, unagi device becomes to faile to play H.264 video. When OMXCodec::freeBuffersOnPort() is called, all gralloc buffers needed to be within OMXCodec or GonkNativeWindow.

Comment 9

[Lucas Adamski [:ladamski]]

We'll definitely take a fix for these 3 related bugs so please don't stop working on them, but we won't hold the release for a camera app crasher that the user can easily work around.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 699791 [details] [diff] [review]
Patch: ImageContainer keeps a previsous Image when rendering via ImageContainerChild

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

We already hold the reference of the images here:
https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ImageContainerChild.cpp#325

And by changing SendFlush() to sync call, we could prevent images being released before RecvReturnImage.

Comment 11

[Sotaro Ikeda [:sotaro]]

After changing SendFlush() to sync call, this abort is still happening. I am going to investingate why  mImageQueue do not work here.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: Abort log of after changing SendFlush() to sync call

Comment 13

[Sotaro Ikeda [:sotaro]]

mikeh, I start to re-think about Part2. attachment 700516 [details] [diff] [review] affect very badly to H.264 video rendering. OMXCodec becomes to abort more often than before. I am going to think about to separate thread for ImageBridgeChild and ImageContainerChild.
 
When OMXCodec::freeBuffersOnPort() is called, all gralloc buffers needed to be within OMXCodec or GonkNativeWindow. attachment 700516 [details] [diff] [review] is going to keep gralloc buffer longer time and is going to fail a buffer status check in OMXCodec::freeBuffersOnPort().

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> mikeh, I start to re-think about Part2. attachment 700516 [details] [diff] [review]
> [review] affect very badly to H.264 video rendering. OMXCodec becomes to
> abort more often than before. I am going to think about to separate thread
> for ImageBridgeChild and ImageContainerChild.
>  
> When OMXCodec::freeBuffersOnPort() is called, all gralloc buffers needed to
> be within OMXCodec or GonkNativeWindow. attachment 700516 [details] [diff] [review]
> [review] is going to keep gralloc buffer longer time and is going to fail a
> buffer status check in OMXCodec::freeBuffersOnPort().

It is a comment for Bug 826829... sorry to post to a different bug.

Comment 15

[Sotaro Ikeda [:sotaro]]

After apply in attachment 700950 [details] [diff] [review] in Bug 826829. I could not re-produce the abort. I assume it is because shutdown sequence happens as expected.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Breaking blocking dependency as bug 826829 is already fixed.

Comment 17

[Sotaro Ikeda [:sotaro]]

This bug is duplicate of bug 826829.