Closed
Bug 987068
Opened 11 years ago
Closed 10 years ago
[Camera] [Nexus4][Madai] preview stream glitch at 1080p
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: wilsonpage, Assigned: shinuk153)
References
Details
Attachments
(3 files, 1 obsolete file)
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•11 years ago
|
||
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?
Flags: needinfo?(sotaro.ikeda.g)
Comment 2•11 years ago
|
||
Hmm I do not know about it. There is gfx layer's tiling. But is is only for thebes layer, not for Image layer.
Flags: needinfo?(sotaro.ikeda.g)
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.
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•11 years ago
|
||
(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.
Flags: needinfo?(mhabicher)
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.
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8412367 -
Attachment is patch: true
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #8412367 -
Flags: review?(mhabicher) → review-
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 11•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 13•11 years ago
|
||
Can you provide the baseline information of this issue?
Flags: needinfo?(wilsonpage)
Reporter | ||
Comment 14•11 years ago
|
||
shinuk: I don't know what 'baseline information' is.
Flags: needinfo?(wilsonpage)
Updated•11 years ago
|
Summary: [Camera] Nexus4 preview stream glitch at 1080p → [Camera] [Nexus4][Madai] preview stream glitch at 1080p
Updated•11 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Assignee | ||
Comment 15•11 years ago
|
||
(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?
Flags: needinfo?(mhabicher)
Comment 16•11 years ago
|
||
(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().
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8412367 -
Attachment is obsolete: true
Flags: needinfo?(dhylands)
Comment 18•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(dhylands)
Assignee | ||
Comment 19•11 years ago
|
||
I don't think we need to change nsGonkCameraControl::SetPreviewSize(). There's no substitution for CAMERA_PARAM_SUPPORTED_PREVIEWSIZES.
Flags: needinfo?(mhabicher)
Updated•10 years ago
|
Comment 21•10 years ago
|
||
(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.
Assignee: nobody → mhabicher
Flags: needinfo?(mhabicher)
Comment 22•10 years ago
|
||
Actually, what I have in mind is more complex than just fixing this issue. Shinuk, I'm handing this back over to you.
Assignee: mhabicher → shinuk153
Updated•10 years ago
|
Comment 23•10 years ago
|
||
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.
Attachment #8420968 -
Flags: review+
Comment 24•10 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Reporter | ||
Comment 25•10 years ago
|
||
mikeh: Should we now re-enable 1080p for capable devices?
Flags: needinfo?(mhabicher)
Comment 26•10 years ago
|
||
(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!
Flags: needinfo?(mhabicher)
You need to log in
before you can comment on or make changes to this bug.
Description
•