1057276 - [Camera] Long press of Home key in preview in video mode causes crash if video and preview sizes are different

Status: RESOLVED WORKSFORME

(Core :: Audio/Video: Recording, defect)

Description

[Vincent Liu[:vliu]]

STR:

1. Launched camera and switched to video mode.
2. Long press the home key to take screenshot for cardview.

Expected result:
Users can see cardview showed on screen.

Actual result:
crash happens

Comment 1

[Vincent Liu[:vliu]]

Attachment: bug-1057276-fix.patch

When Gecko called videoImage->SetData(data) for the incoming preview stream, width/hight in data it filled was determined by mCurrentConfiguration.mPreviewSize. For the case that video size and preview are different, mCurrentConfiguration.mPreviewSize will be set to video size. 

  Once it is set as video size, width/hight would not match with the values in GraphicBuffer operated under NativeWindow. The crash happens when system app wanted to convert YUV data to RGB for generating cardview. 

  Based on this, my idea is Gecko don't modify mCurrentConfiguration.mPreviewSize by mLastRecorderSize in SetConfigurationInternal(). It should be determined only when Gecko called SetPreviewSize(). Would you please have some comments for my patch? Thanks

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Vincent, under what circumstances are the preview and video sizes different?

Justin, I thought the Camera app currently uses the same size for both of these settings.

Bug 1014877 has been filed to track decoupling preview and video-recording frame sizes.

Comment 3

[Justin D'Arcangelo [:justindarc]]

(In reply to Mike Habicher [:mikeh] from comment #2)
> Vincent, under what circumstances are the preview and video sizes different?
> 
> Justin, I thought the Camera app currently uses the same size for both of
> these settings.
> 
> Bug 1014877 has been filed to track decoupling preview and video-recording
> frame sizes.

I'm not sure. I think this is referring to the size of the <video> element vs. the preview source size.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #3)

> I'm not sure. I think this is referring to the size of the <video> element
> vs. the preview source size.

My reading was that Gecko was getting different sizes for preview and for the recorded video frames, which I thought was something we didn't try to do right now. Maybe vliu can clarify.

Comment 5

[Vincent Liu[:vliu]]

(In reply to Mike Habicher [:mikeh] from comment #2)
> Vincent, under what circumstances are the preview and video sizes different?
> 
> Justin, I thought the Camera app currently uses the same size for both of
> these settings.
> 
> Bug 1014877 has been filed to track decoupling preview and video-recording
> frame sizes.

preview/video sizes uses CAMERA_PARAM_SUPPORTED_PREVIEWSIZES/CAMERA_PARAM_SUPPORTED_VIDEOSIZES to get their parameters separately so I believe it has chance they got different values. Actually I found they are different when I looked into Bug 1050192. That's the reason why I filed this bug. 

When I saw the functions SetPreviewSize() and SetVideoSize(), I thought the purpose of these two functions were implemented by setting preview size and video size separately. No matter whether camera app uses the same size size or not, Gecko should deal with them into different case. That's also helpful if 3rd app wants to implement camera function.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Vincent Liu[:vliu] from comment #5)
> preview/video sizes uses
> CAMERA_PARAM_SUPPORTED_PREVIEWSIZES/CAMERA_PARAM_SUPPORTED_VIDEOSIZES to get
> their parameters separately so I believe it has chance they got different
> values. Actually I found they are different when I looked into Bug 1050192.
> That's the reason why I filed this bug. 
> 
> When I saw the functions SetPreviewSize() and SetVideoSize(), I thought the
> purpose of these two functions were implemented by setting preview size and
> video size separately. No matter whether camera app uses the same size size
> or not, Gecko should deal with them into different case. That's also helpful
> if 3rd app wants to implement camera function.

Vincent: As Mike already mentioned, Bug 1014877 is for breaking the coupling that currently exists between the preview size and video size. Currently, it is not possible for us to specify a preview size that is different than the video size in the current recorder profile.

Comment 7

[Vincent Liu[:vliu]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #6)
> (In reply to Vincent Liu[:vliu] from comment #5)
> > preview/video sizes uses
> > CAMERA_PARAM_SUPPORTED_PREVIEWSIZES/CAMERA_PARAM_SUPPORTED_VIDEOSIZES to get
> > their parameters separately so I believe it has chance they got different
> > values. Actually I found they are different when I looked into Bug 1050192.
> > That's the reason why I filed this bug. 
> > 
> > When I saw the functions SetPreviewSize() and SetVideoSize(), I thought the
> > purpose of these two functions were implemented by setting preview size and
> > video size separately. No matter whether camera app uses the same size size
> > or not, Gecko should deal with them into different case. That's also helpful
> > if 3rd app wants to implement camera function.
> 
> Vincent: As Mike already mentioned, Bug 1014877 is for breaking the coupling
> that currently exists between the preview size and video size. Currently, it
> is not possible for us to specify a preview size that is different than the
> video size in the current recorder profile.

Sorry about not mentioning it clearly. When I looked into Bug 1050192, it run on 2.0 branch. From adding log printing before the below line, I got

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/293fddaf1a92/dom/camera/GonkCameraControl.cpp#l1291

best.height=320, best.width=480, mLastRecorderSize.width=640, mLastRecorderSize.height=480

mLastRecorderSize overwrote mCurrentConfiguration.mPreviewSize when nsGonkCameraControl::SetConfigurationInternal was called. After that, when SetData was called, crash happened.

Hi Justin, as you mentioned that it is not possible to specify a preview size that is different than the video size in the current recorder profile. Did you said it on master branch? Does it also valid on v2.0 branch?

Comment 8

[Vincent Liu[:vliu]]

[Blocking Requested - why for this release]:

Comment 9

[Justin D'Arcangelo [:justindarc]]

(In reply to Vincent Liu[:vliu] from comment #7)
> Sorry about not mentioning it clearly. When I looked into Bug 1050192, it
> run on 2.0 branch. From adding log printing before the below line, I got
> 
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/293fddaf1a92/dom/
> camera/GonkCameraControl.cpp#l1291
> 
> best.height=320, best.width=480, mLastRecorderSize.width=640,
> mLastRecorderSize.height=480
> 
> mLastRecorderSize overwrote mCurrentConfiguration.mPreviewSize when
> nsGonkCameraControl::SetConfigurationInternal was called. After that, when
> SetData was called, crash happened.
> 
> Hi Justin, as you mentioned that it is not possible to specify a preview
> size that is different than the video size in the current recorder profile.
> Did you said it on master branch? Does it also valid on v2.0 branch?

AFAIK, we are unable to specify separate values for the preview and video recording sizes in both v2.0 and master. The bug 1014877 that Mike was referring to was to allow us to specify different sizes in the JS application code. Until that lands, however, I do not think it is even possible for us to configure it that way. Perhaps Mike could confirm?

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

AFAIK, the Camera app has never used different preview and video frame sizes. And if it were to do so now, it would definitely run into issues. Hence bug 1014877.

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

vliu, what exactly is the issue here? The STR in comment 0 doesn't cause a crash on Flame. If you're seeing this on a specific build/platform, then please include that information.

Otherwise, let's close this bug as a dupe of bug 1014877.

Comment 12

[Vincent Liu[:vliu]]

(In reply to Mike Habicher [:mikeh] from comment #11)
> vliu, what exactly is the issue here? The STR in comment 0 doesn't cause a
> crash on Flame. If you're seeing this on a specific build/platform, then
> please include that information.
> 
> Otherwise, let's close this bug as a dupe of bug 1014877.

Flame got the same video/preview size so it doesn't causes any problem. After looked into bug 1014877, I believe it is a dup of this bug. I will close this bug and then set appropriate flag to bug 1014877.

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8477296 [details] [diff] [review]
bug-1057276-fix.patch

Clearing r? flag since the fix for this issue landed in bug 1014877.

Comment 14

[Vincent Liu[:vliu]]

I reopen it to track the issue for partner device.

Comment 15

[Vincent Liu[:vliu]]

Hi Francis,
We need the patch of bug 1014877 to fix this issue on woodduck. Thanks

Comment 16

[howie [:howie]]

triage: blocking.

Comment 17

[Keven Kuo [:kkuo]]

Hi! Vincent,

Is this also occurring on Flame device?
Please fine someone to review your patch. Thanks

--
Keven

Comment 18

[Vincent Liu[:vliu]]

(In reply to Keven Kuo [:kkuo] from comment #17)
> Hi! Vincent,
> 
> Is this also occurring on Flame device?
> Please fine someone to review your patch. Thanks
> 
> --
> Keven

Hi Keven,

I'd checked with Camera Owner, and actually he has another bug which can fix this issue in his backlog. Please see bug 1014877 for detail. Currently there is v2 patch in bug 1014877 which also contains the fix I attached in this bug. I'd tried to merge this v2 patch into v2.0m branch, and it can fix the problem in partner device. Bug 1014877 has marked as 2.0M+ so I believe this issue can be fixed by 1014877.

Comment 19

[Francis Lee [:frlee]]

per comment 16, already changed to 2.0m+

Comment 20

[Josh Cheng [:josh]]

Hi Hubert,
Can you kindly verify whether the bug is fix? Since its dependency bug 1014877 is fixed.
Thank you!

Comment 21

[Evelyn Hung]

Hi Josh,
I do think bug 1014877 and this bug should be 2.0+, not 2.0M. I've updated a few cases like this to 2.0+, but I'm not pretty sure on this one. What do you think?

Comment 22

[Josh Cheng [:josh]]

Hi Hubert,

Can you kindly verify whether the bug is fix in 2.0 and 2.0M? 
It's dependency bug 1014877 is fixed for 2.0.

Thank you!

Comment 23

[Hubert Lu[:hlu] <[e-mail]>]

Could NOT reproduce this issue on Woodduck v2.0m and Flame v2.0 

== Build Information - Flame v2.0 ==
Gaia-Rev        9725d188a733a4aeebcfcf4c52d28e1ad8a2ba6f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/05c6a4fed6bc
Build ID        20141001174325
Version         32.0
Device Name     flame
FW-Release      4.4.2
FW-Incremental  27
FW-Date         Thu Sep  4 14:59:02 CST 2014
Bootloader      L1TC10011800

== Build Information - Woodduck v2.0m ==
Gaia-Rev        a7f23ca4d207d862bcf9681176e5e1ff92f06ea0
Gecko-Rev       1bbfa5da30c687c88b7542a5b1b8724a14762b34
Build ID        20141003101613
Version         32.0
Device Name     soul35
FW-Release      4.4.2
FW-Incremental  1412301835
FW-Date         Fri Oct  3 10:04:14 CST 2014