987068 - [Camera] [Nexus4][Madai] preview stream glitch at 1080p

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Wilson Page [:wilsonpage]]

Attachment: nexus4-1080p.png

When the camera recorderProfile is set to 1080p there are glitches in the preview stream (see attached screenshot).

STEPS:

1. Uncomment the 1080p option in `recorderProfilesBack` in apps/camera/js/config/app.js
2. Launch camera app
3. Switch to 'video' mode
4. Observe broken preview stream

Comment 1

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

It looks like the viewfinder frame is getting tiled somehow. I don't know much about the app's structure, but are we sure this isn't some CSS weirdness?

Sotaro, does the attachment look like a known issue?

Comment 2

[Sotaro Ikeda [:sotaro]]

Hmm I do not know about it. There is gfx layer's tiling. But is is only for thebes layer, not for Image layer.

Comment 3

[hyuna.cho]

Attachment: 720p_resoution.png

It's reproduced in my other android devices.
This device support the preview size upto 960x540.
When the video resolution set to 720p or 1080p, the preview size set to 960x540.
The crash screen is displayed when set to only 720p resolution.
I attached the screenshot.

Comment 4

[shinuk]

OnNewPreviewFrame of nsGonkCameraControl,

videoImage size is set to the recorded video size not an actual preview frame size.

To send data to preview display path not an encoding path to media recorder,

I guess mPreviewSize.width and mPreviewSize.height are supposed to set to actual preview size.

Comment 5

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

(In reply to shinuk from comment #4)
> 
> I guess mPreviewSize.width and mPreviewSize.height are supposed to set to
> actual preview size.

Hi shinuk,

The video recording path currently makes a couple of assumptions about preview.size == video_recording.size. If you have a proposed patch, I'd be happy to take a look at it.

Comment 6

[shinuk]

Attachment: duplicated preview size setting

Comment 7

[shinuk]

I was wondering if this code block can be removed.
Because this restores preview size after supported preview size has been set.

if (aConfig.mMode == kVideoMode) {
  mCurrentConfiguration.mPreviewSize = mLastRecorderSize;
}

Also, mLastRecorderSize is video size not preview size.
Please find attached patch.

Comment 8

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

(In reply to shinuk from comment #7)
>
> I was wondering if this code block can be removed.
> Because this restores preview size after supported preview size has been set.
> 
> if (aConfig.mMode == kVideoMode) {
>   mCurrentConfiguration.mPreviewSize = mLastRecorderSize;
> }

That's part of the fix, but I think GonkCameraParameters will need to change as well. See this:

http://androidxref.com/4.0.4/xref/frameworks/base/include/camera/CameraParameters.h#63

GonkCameraParameters::Initialize()[1] will need to pull CAMERA_PARAM_SUPPORTED_VIDEOSIZES and check to see if the list is empty. If it is, it could set a flag that is then used to either a) map Get/SetTranslated()[2] calls to CAMERA_PARAM_VIDEOSIZE to _PREVIEWSIZE, or b) make Get/SetTranslated() calls to _VIDEOSIZE return NS_ERROR_NOT_AVAILABLE.

I think my preference is for the latter. Since this is more complex than just (re)mapping an obsolete parameter name, I think it makes sense to handle that specific return value (and redirect calls to _PREVIEWSIZE) in GonkCameraControl::Get/Set().

1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraParameters.cpp#192
2. http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraParameters.cpp#281

Comment 9

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

Comment on attachment 8412367 [details] [diff] [review]
duplicated preview size setting

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

r-, please see feedback in comment 8.

Comment 10

[shinuk]

(In reply to Mike Habicher [:mikeh] from comment #8)

> GonkCameraParameters::Initialize()[1] will need to pull
> CAMERA_PARAM_SUPPORTED_VIDEOSIZES and check to see if the list is empty. If
> it is, it could set a flag that is then used to either a) map
> Get/SetTranslated()[2] calls to CAMERA_PARAM_VIDEOSIZE to _PREVIEWSIZE, or
> b) make Get/SetTranslated() calls to _VIDEOSIZE return
> NS_ERROR_NOT_AVAILABLE.

We can check supported video sizes through 'readonly attribute sequence<CameraSize> videoSizes' in CameraCapabilities.webidl. This attribute checks to see if the supported size table is empty, if it is this gets _SUPPORTED_PREVIEWSIZES from nsGonkCameraControl::Get method.
I think we can do this in Gaia layer but even if we have checked the supported video size and preview size already, attached code block changes mPreviewSize of current configuration after that.

Because we knew the sensor specification and the supported video sizes table,
I assume that to make Get/SetTranslated() calls to _VIDEOSIZE return NS_ERROR_NOT_AVAILABLE is not my preference. I guess the preview size of video configuration is not proper.

Comment 11

[shinuk]

This issue is reproducible with Madai.
Can you take a look and see if this causes preview stream glitch?

  data.mGraphicBuffer = static_cast<layers::GraphicBufferLocked*>(aBuffer);
  data.mPicSize = IntSize(mCurrentConfiguration.mPreviewSize.width,
                          mCurrentConfiguration.mPreviewSize.height);
  videoImage->SetData(data);

At this point mPreviewSize is video size. Attached patch blocks to modify mPreviewSize from changing to video size.

Comment 12

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

shinuk, I agree that your patch will fix Madai, but it will likely break other devices without the changes I recommended in comment 8.

I would rather not have the logic to handle this in Gaia as you recommend in comment 10. This exposes the underlying Androidism to the Web, which we are doing our best to hide.

Comment 13

[shinuk]

Can you provide the baseline information of this issue?

Comment 14

[Wilson Page [:wilsonpage]]

shinuk: I don't know what 'baseline information' is.

Comment 15

[shinuk]

(In reply to Mike Habicher [:mikeh] from comment #12)
> shinuk, I agree that your patch will fix Madai, but it will likely break
> other devices without the changes I recommended in comment 8.
> 
> I would rather not have the logic to handle this in Gaia as you recommend in
> comment 10. This exposes the underlying Androidism to the Web, which we are
> doing our best to hide.

If my patch breaks your device, can you please make a comment with a log file?

Comment 16

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

(In reply to shinuk from comment #15)
> 
> If my patch breaks your device, can you please make a comment with a log
> file?

Sorry for the late response. I've looked through the code and am satisfied that your patch will work for devices that support CAMERA_PARAM_SUPPORTED_VIDEOSIZES. To make sure we don't break other devices, your patch also needs to change nsGonkCameraControl::SetPreviewSize() and SetVideoSize() to call Get(CAMERA_PARAM_SUPPORTED_...) instead of mParams.Get(). This will call nsGonkCameraControl::Get() which properly substitutes _PREVIEWSIZES for _VIDEOSIZES.

I will be out of the office next week; if you post a new patch during that time, please ask dhylands to review it.

Dave, you can validate the logic against this: http://androidxref.com/4.4.2_r2/xref/frameworks/av/include/camera/CameraParameters.h#62 - in particular, the comments regarding get/setVideoSize() and getSupportedVideoSizes().

Comment 17

[shinuk]

Attachment: preview stream glitch patch

Comment 18

[Dave Hylands [:dhylands]]

Comment on attachment 8420968 [details] [diff] [review]
preview stream glitch patch

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

Sorry for the delay - I was at a work week and this slipped through the cracks.

I think Mike will need to look at this. My reading says that this still isn't quite doing the right thing.

::: dom/camera/GonkCameraControl.cpp
@@ +1220,5 @@
>  {
>    MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread);
>  
>    nsTArray<Size> previewSizes;
> +  nsresult rv = Get(CAMERA_PARAM_SUPPORTED_PREVIEWSIZES, previewSizes);

So I'm still confused. According to http://androidxref.com/4.4.2_r2/xref/frameworks/av/include/camera/CameraParameters.h#63 it says that you shouldn't even call setVideoSize if getSupportedVideoSizes returns an empty list.

Calling Get here instead of mParams.Get will cause us to get a "nice" answer, but I don't see how we'll wind up not calling mParams.Set below.

Comment 19

[shinuk]

I don't think we need to change nsGonkCameraControl::SetPreviewSize().
There's no substitution for CAMERA_PARAM_SUPPORTED_PREVIEWSIZES.

Comment 21

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

(In reply to Dave Hylands [:dhylands] (away - back May 16) from comment #18)
> 
> Calling Get here instead of mParams.Get will cause us to get a "nice"
> answer, but I don't see how we'll wind up not calling mParams.Set below.

This patch will work, since setVideoSize() is just a pretty wrapper for CameraParameters::set(KEY_VIDEO_SIZE, ...). In general, if the camera library doesn't support a parameter, it ignores it. So if KEY_SUPPORTED_VIDEO_SIZES and getSupportedVideoSizes() return nothing, the all to mParams.Set() will have no effect.

In this case, the other call to mParams.Set(KEY_PREVIEW_SIZE, ...) will be the one that sets the frame size for the recorded video.

That said, I think we can improve this patch a bit. I'm going to yoink it.

Comment 22

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

Actually, what I have in mind is more complex than just fixing this issue. Shinuk, I'm handing this back over to you.

Comment 23

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

Comment on attachment 8420968 [details] [diff] [review]
preview stream glitch patch

This patch will fix this specific issue for now, but I've created bug 1014877 to track a more future-proof-y solution.

Comment 24

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

I'm going to close this as the parameter-access change was subsumed into bug 1025197, and dealing with split resolutions for video recording has moved to bug 1014877.

Comment 25

[Wilson Page [:wilsonpage]]

mikeh: Should we now re-enable 1080p for capable devices?

Comment 26

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

(In reply to Wilson Page [:wilsonpage] from comment #25)

> mikeh: Should we now re-enable 1080p for capable devices?

I don't see why not!