Closed
Bug 1014877
Opened 10 years ago
Closed 10 years ago
[Camera][Gecko] Uncouple preview and video frame size dependencies
Categories
(Firefox OS Graveyard :: Gaia::Camera, enhancement)
Tracking
(blocking-b2g:2.0M+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)
People
(Reporter: mikeh, Assigned: mikeh)
References
Details
(Whiteboard: [priority])
Attachments
(2 files, 6 obsolete files)
(deleted),
patch
|
aosmond
:
review+
shinuk153
:
feedback+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
CameraControl makes a lot of assumptions that the preview frame size and the recorded video frame size are the same. Although the current Camera app plays along, this may not be the case going forward, so it would be good to separate these settings.
Once done, a 360x480 phone could record 1080p video without having to shuttle all those extra pixels to the display.
My test platform provides
supported preview sizes:
960x540,800x480,800x800,720x720,640x480,480x320,352x288,320x240,240x160,176x144
supported video sizes:
1920x1080,1280x720,960x540,864x480,800x480,720x480,640x480,480x320,352x288,320x240,176x144,144x176
Once video profile(recorderProfile) has been set to '1080p' from application, the preview size is set to '800x800' within GonkCameraControl.
But, the best supported preview size for 1920x1080 is 960x540, because those are the same ratio.
NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined:
Testing completed:
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:
Attachment #8463823 -
Flags: approval-mozilla-b2g32?
Attachment #8463823 -
Flags: approval-mozilla-b2g30?
Attachment #8463823 -
Flags: approval-mozilla-b2g28?
Flags: needinfo?(mhabicher)
Attachment #8463823 -
Flags: approval-mozilla-b2g30?
Attachment #8463823 -
Flags: approval-mozilla-b2g28?
Comment 3•10 years ago
|
||
Comment on attachment 8463823 [details] [diff] [review]
SupportedSize_patch
b2g32-
This patch has not been reviewed. It also hasn't landed on mozilla-central yet. Please request review before requesting uplift. As well, for b2g32 you should set the blocking 2.0? flag at this point in the schedule rather than request uplift approval.
Attachment #8463823 -
Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32-
Attachment #8463823 -
Flags: review?(mhabicher)
[Blocking Requested - why for this release]:
GonkCameraControl doesn't map exact preview size and need to separate recording video size and preview frame size.
blocking-b2g: --- → 2.0?
Comment 5•10 years ago
|
||
shinuk - Before considering this for 2.0 blocking, can you please comment on the user impact of this bug?
Flags: needinfo?(shinuk153)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8463823 [details] [diff] [review]
SupportedSize_patch
Review of attachment 8463823 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/camera/GonkCameraControl.cpp
@@ +1335,3 @@
> for (nsTArray<Size>::index_type i = 0; i < supportedSizes.Length(); i++) {
> Size size = supportedSizes[i];
> + uint32_t delta = abs((long int)(size.width - aSize.width)) + abs((long int)(size.height - aSize.height));
Why this change?
Comment 7•10 years ago
|
||
Shinuk,
Please renom with the user impact and justification on why this needs to be considered for 2.0 this late in the cycle (we are well past feature complete). Please see: https://wiki.mozilla.org/B2G/Triage on nomination notes and blocking criteria.
This work is in our camera backlog. We do not currently support separating viewfinder resolution with recorded video resolution in camera control api.
Thanks
Hema
Severity: normal → enhancement
blocking-b2g: 2.0? → ---
Updated•10 years ago
|
blocking-b2g: --- → backlog
(In reply to Mike Habicher [:mikeh] from comment #6)
> Comment on attachment 8463823 [details] [diff] [review]
> SupportedSize_patch
>
> Review of attachment 8463823 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/camera/GonkCameraControl.cpp
> @@ +1335,3 @@
> > for (nsTArray<Size>::index_type i = 0; i < supportedSizes.Length(); i++) {
> > Size size = supportedSizes[i];
> > + uint32_t delta = abs((long int)(size.width - aSize.width)) + abs((long int)(size.height - aSize.height));
>
> Why this change?
My platform provides supported pic sizes: 3264x2448,3264x1836,3200x2400,3200x1920,2592x1944,2560x1920,2448x2448,... etc.
When I select 2448x2448 for 1:1 ratio,
targetArea = 3264x1836 == 2448x2448 (uint32_t targetArea = aSize.width * aSize.height;)
CameraControl sets picture size to 3264x1836 instead of 2448x2448 in order of index.
So that I made a change to get exact width and height first, and to get the closest supported size next.
Flags: needinfo?(shinuk153)
Flags: needinfo?(mhabicher)
(In reply to Hema Koka [:hema] from comment #7)
I understand that we currently support separating viewfinder resolution with recorded video resolution in camera control api.
> /* Pre-emptive camera configuration options. */
> dictionary CameraConfiguration
> {
> CameraMode mode = "picture";
> CameraSize previewSize = null;
> DOMString recorderProfile = "cif"; // or some other recording profile
> // supported by the CameraControl
> };
e.g. previewSize = 960x540, recorderProfile = "720p"(1280x720)
Since we could set previewSize and recorderProfile separately,
I think we support separating viewfinder resolution with recorded video resolution.
Flags: needinfo?(mhabicher)
Flags: needinfo?(hkoka)
Comment 10•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #5)
the user impact of this bug:
CameraControl possibly maps user requested picture size onto inappropriate picture size.
Flags: needinfo?(lmandel)
Comment 11•10 years ago
|
||
Attachment #8463823 -
Attachment is obsolete: true
Attachment #8463823 -
Flags: review?(mhabicher)
Attachment #8468150 -
Flags: review?(mhabicher)
Updated•10 years ago
|
Flags: needinfo?(lmandel)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8468150 [details] [diff] [review]
Uncouple_preview_video_size
Review of attachment 8468150 [details] [diff] [review]:
-----------------------------------------------------------------
shinuk, this needs an automated test. There are a number of approaches you can take -- look in dom/camera/test. One supported feature is the ability to inject artificial CameraParameters.
Attachment #8468150 -
Flags: review?(mhabicher) → review-
Comment 13•10 years ago
|
||
Moving this to 2.1 release milestone.
Flags: needinfo?(hkoka)
Target Milestone: --- → 2.1 S3 (29aug)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(mhabicher)
Comment 14•10 years ago
|
||
shinuk,
Are you addressing the review comments from Mike: https://bugzilla.mozilla.org/show_bug.cgi?id=1014877#c12? Please provide an update?
Thanks much!
Hema
Flags: needinfo?(shinuk153)
Comment 15•10 years ago
|
||
> Created attachment 8468150 [details] [diff] [review]
> Uncouple_preview_video_size
This patch causes another issue in case switching preview resolutions between photo and video mode.
Between different preview ratios, preview frames are sometimes flickering.
Better not to apply this patch for the present to prevent UI issues.
I'd rather file another bug only for the supported preview and image size fixes.
Bug 1054803.
Flags: needinfo?(shinuk153)
Comment 17•10 years ago
|
||
[Blocking Requested - why for this release]:
Partner device runs on v2.0 branch needs this patch to fix the problem.
blocking-b2g: backlog → 2.0?
Updated•10 years ago
|
Flags: needinfo?(vliu)
Comment 18•10 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #17)
> [Blocking Requested - why for this release]:
>
> Partner device runs on v2.0 branch needs this patch to fix the problem.
vliu: Need more info on what issue we are seeing on the partner device before we can decide on blocking for 2.0
Comment 19•10 years ago
|
||
Originally the issue I took is Bug 1050192. In bug 1050192 comment 18, I also wrote
==
1. This crash issue happens when long press the home key under the preview in picture mode. Currently Peter has offered a patch to fix this issue. I am helping to verify.
2. It also expose another question. Since I think it belong to common issue, I filed Bug 1057276 to discuss.
==
Since I had found bug 1050192 contains two different questions, I filed Bug 1057276 for the second one. I also had some expressions in Bug 1057267 comment 1 to detail the issue. After had some discussions with :mikeh in Bug 1057276, I think it is a dup of this bug.
Based on the above tracking flow, that's why I marked this bug as "2.0?".
Flags: needinfo?(vliu)
Comment 20•10 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #20)
> 1057276 for the second one. I also had some expressions in Bug 1057267
> comment 1 to detail the issue. After had some discussions with :mikeh in Bug
Oops. Typo error. The expressions should link to Bug 1057276 comment 1.
Comment 21•10 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #20)
> Originally the issue I took is Bug 1050192. In bug 1050192 comment 18, I
> also wrote
>
> ==
> 1. This crash issue happens when long press the home key under the preview
> in picture mode. Currently Peter has offered a patch to fix this issue. I am
> helping to verify.
>
> 2. It also expose another question. Since I think it belong to common issue,
> I filed Bug 1057276 to discuss.
> ==
>
> Since I had found bug 1050192 contains two different questions, I filed Bug
> 1057276 for the second one. I also had some expressions in Bug 1057267
> comment 1 to detail the issue. After had some discussions with :mikeh in Bug
> 1057276, I think it is a dup of this bug.
>
> Based on the above tracking flow, that's why I marked this bug as "2.0?".
Vincent,
Sorry, I am a bit confused. As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1057276#c9 and https://bugzilla.mozilla.org/show_bug.cgi?id=1057276#c10, we do not support having different preview and video recording sizes for 2.0 or master. So not sure how we are getting to a state of different preview/video size
What 2.0 devices are you seeing the crash on, as originally reported here https://bugzilla.mozilla.org/show_bug.cgi?id=1057276?
Flags: needinfo?(vliu)
Flags: needinfo?(mhabicher)
Updated•10 years ago
|
Target Milestone: 2.1 S3 (29aug) → ---
Comment 22•10 years ago
|
||
(In reply to Hema Koka [:hema] from comment #22)
> Vincent,
>
> Sorry, I am a bit confused. As per comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=1057276#c9 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1057276#c10, we do not support
> having different preview and video recording sizes for 2.0 or master. So not
> sure how we are getting to a state of different preview/video size
>
I tried to add log info to observe best in
[1]: https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/de70f9a40834/dom/camera/GonkCameraControl.cpp#l1291
[2]: https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/de70f9a40834/dom/camera/GonkCameraControl.cpp#l1314
In picture mode:
I/camera ( 2000): SetPreviewSize. best.width=384, best.height=288
I/camera ( 2000): SetVideoSize. best.width=320, best.height=240
In video mode:
I/camera ( 2000): SetPreviewSize. best.width=480, best.height=320
I/camera ( 2000): SetVideoSize. best.width=640, best.height=480
I am not sure it the above info is enough or not. If you need me to offer other info, I can do it.
> What 2.0 devices are you seeing the crash on, as originally reported here
> https://bugzilla.mozilla.org/show_bug.cgi?id=1057276?
Actually you can see Bug 1050192 for the crash issue I started.
Flags: needinfo?(vliu)
Comment 23•10 years ago
|
||
Vincent - While this could reproduce in theory on 2.0, we currently do not have any production proof of this bug happening on 2.0 devices outside of Woodduck. As such, I think we should move this to 2.0M? until we have proof of this happening on a 2.0 device outside of Woodduck. Does that make sense?
Flags: needinfo?(vliu)
Comment 24•10 years ago
|
||
Triage Decision on 9/10: Removing from 2.0 nom since it is not clear of any impact on 2.0 targeted devices.
If this is for 2.0M, please open a separate bug and track changes required for it. The original enhancement/changes that Mike describes will be looked into in a future release
blocking-b2g: 2.0? → backlog
Updated•10 years ago
|
Whiteboard: [priority]
Comment 25•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #24)
> Vincent - While this could reproduce in theory on 2.0, we currently do not
> have any production proof of this bug happening on 2.0 devices outside of
> Woodduck. As such, I think we should move this to 2.0M? until we have proof
> of this happening on a 2.0 device outside of Woodduck. Does that make sense?
Agree. It's more safer for 2.0 branch at the time of this moment.
(In reply to Hema Koka [:hema] from comment #25)
> Triage Decision on 9/10: Removing from 2.0 nom since it is not clear of any
> impact on 2.0 targeted devices.
>
> If this is for 2.0M, please open a separate bug and track changes required
> for it. The original enhancement/changes that Mike describes will be looked
> into in a future release
Apologized for switching |blocking-b2g| to |2.0?| since I thought Bug 1057276 is a dup of this one. I will track the issue I saw by Bug 1057276.
Flags: needinfo?(vliu)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mhabicher
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 26•10 years ago
|
||
Shinuk, try this out. It uncouples the preview size and the video recording size.
Attachment #8468150 -
Attachment is obsolete: true
Attachment #8488151 -
Flags: feedback?(shinuk153)
Comment 27•10 years ago
|
||
Comment on attachment 8488151 [details] [diff] [review]
Uncouple preview and video sizes, v1
looks good to me, but applying this change can make first video frame flickering sometimes if preview size is larger than the original video frames size.
I think the difference between video and preview size is not the cause of Bug 1057276.
If preview frame looks proper, the crash of card view is usually caused by display layer.
Attachment #8488151 -
Flags: feedback?(shinuk153) → feedback+
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to shinuk from comment #28)
> looks good to me, but applying this change can make first video frame
> flickering sometimes if preview size is larger than the original video
> frames size.
Thanks for the feedback. Can you attach a video showing the flickering please? I don't see if here.
Flags: needinfo?(shinuk153)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8488151 [details] [diff] [review]
Uncouple preview and video sizes, v1
Andrew, when you review this, can you please compare it with the AOSP "spec" here[1] to make sure I didn't miss anything? Thanks.
1. http://androidxref.com/4.4.4_r1/xref/frameworks/av/include/camera/CameraParameters.h#63
Attachment #8488151 -
Flags: review?(aosmond)
Comment 30•10 years ago
|
||
Comment on attachment 8488151 [details] [diff] [review]
Uncouple preview and video sizes, v1
Review of attachment 8488151 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like what you've changed is fine but I think you need some other changes:
1) nsGonkCameraControl::SetVideoSize can in theory trigger Set(CAMERA_PARAM_VIDEOSIZE) even if mSeparateVideoAndPreviewSizesSupported is false (remember it substitutes the earlier Get(CAMERA_PARAM_SUPPORTED_VIDEOSIZES) for Get(CAMERA_PARAM_SUPPORTED_PREVIEWSIZES) if it isn't supported) because of the last sanity check in nsGonkCameraControl::SetPreviewSize [1].
2) nsGonkCameraControl::SetVideoSize always updates mLastRecorderSize even if Set(CAMERA_PARAM_VIDEOSIZE) fails [2]. Also, if mSeparateVideoAndPreviewSizesSupported is false, shouldn't this value track the preview size?
[1] http://hg.mozilla.org/integration/b2g-inbound/file/b75ec660eec1/dom/camera/GonkCameraControl.cpp#l1275
[2] http://hg.mozilla.org/integration/b2g-inbound/file/b75ec660eec1/dom/camera/GonkCameraControl.cpp#l1304
Attachment #8488151 -
Flags: review?(aosmond) → review-
Assignee | ||
Comment 31•10 years ago
|
||
Incorporate review feedback.
Shinuk, can you please test this patch as well? It changes a few things.
Attachment #8488151 -
Attachment is obsolete: true
Attachment #8489445 -
Flags: review?(aosmond)
Attachment #8489445 -
Flags: feedback?(shinuk153)
Updated•10 years ago
|
Attachment #8489445 -
Flags: review?(aosmond) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Patch tested on Flame, works properly.
Try-server push: https://tbpl.mozilla.org/?tree=Try&rev=97cf44dbba8b
Comment 33•10 years ago
|
||
Flags: needinfo?(shinuk153)
Comment 34•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #29)
> Thanks for the feedback. Can you attach a video showing the flickering
> please? I don't see if here.
please find the attached video showing flickering during preview resolution change.
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to shinuk from comment #35)
> please find the attached video showing flickering during preview resolution
> change.
Thank you for the video. We don't see that size glitch happening on our reference device. What code (gecko and gaia) are you running?
Status: NEW → ASSIGNED
Comment 36•10 years ago
|
||
Comment on attachment 8489445 [details] [diff] [review]
Uncouple preview and video sizes, v2
>diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
>--- a/dom/camera/GonkCameraControl.cpp
>+++ b/dom/camera/GonkCameraControl.cpp
>@@ -64,16 +64,17 @@ nsGonkCameraControl::nsGonkCameraControl
> : CameraControlImpl(aCameraId)
> , mLastPictureSize({0, 0})
> , mLastThumbnailSize({0, 0})
> , mPreviewFps(30)
> , mResumePreviewAfterTakingPicture(false) // XXXmikeh - see bug 950102
> , mFlashSupported(false)
> , mLuminanceSupported(false)
> , mAutoFlashModeOverridden(false)
>+ , mSeparateVideoAndPreviewSizesSupported(false)
> , mDeferConfigUpdate(0)
> , mMediaProfiles(nullptr)
> , mRecorder(nullptr)
> , mRecorderMonitor("GonkCameraControl::mRecorder.Monitor")
> , mProfileManager(nullptr)
> , mRecorderProfile(nullptr)
> , mVideoFile(nullptr)
> , mReentrantMonitor("GonkCameraControl::OnTakePicture.Monitor")
>@@ -162,17 +163,16 @@ nsGonkCameraControl::Initialize()
> int areas;
> mParams.Get(CAMERA_PARAM_SUPPORTED_MAXMETERINGAREAS, areas);
> mCurrentConfiguration.mMaxMeteringAreas = areas != -1 ? areas : 0;
> mParams.Get(CAMERA_PARAM_SUPPORTED_MAXFOCUSAREAS, areas);
> mCurrentConfiguration.mMaxFocusAreas = areas != -1 ? areas : 0;
>
> mParams.Get(CAMERA_PARAM_PICTURE_SIZE, mLastPictureSize);
> mParams.Get(CAMERA_PARAM_PREVIEWSIZE, mCurrentConfiguration.mPreviewSize);
>- mParams.Get(CAMERA_PARAM_VIDEOSIZE, mLastRecorderSize);
>
> nsString luminance; // check for support
> mParams.Get(CAMERA_PARAM_LUMINANCE, luminance);
> mLuminanceSupported = !luminance.IsEmpty();
>
> nsString flashMode;
> mParams.Get(CAMERA_PARAM_FLASHMODE, flashMode);
> mFlashSupported = !flashMode.IsEmpty();
>@@ -186,27 +186,37 @@ nsGonkCameraControl::Initialize()
> mLastPictureSize.width, mLastPictureSize.height);
> DOM_CAMERA_LOGI(" - default picture file format: %s\n",
> NS_ConvertUTF16toUTF8(mFileFormat).get());
> DOM_CAMERA_LOGI(" - default picture quality: %f\n", quality);
> DOM_CAMERA_LOGI(" - default thumbnail size: %u x %u\n",
> mLastThumbnailSize.width, mLastThumbnailSize.height);
> DOM_CAMERA_LOGI(" - default preview size: %u x %u\n",
> mCurrentConfiguration.mPreviewSize.width, mCurrentConfiguration.mPreviewSize.height);
>- DOM_CAMERA_LOGI(" - default video recorder size: %u x %u\n",
>- mLastRecorderSize.width, mLastRecorderSize.height);
> DOM_CAMERA_LOGI(" - luminance reporting: %ssupported\n",
> mLuminanceSupported ? "" : "NOT ");
> if (mFlashSupported) {
> DOM_CAMERA_LOGI(" - flash: supported, default mode '%s'\n",
> NS_ConvertUTF16toUTF8(flashMode).get());
> } else {
> DOM_CAMERA_LOGI(" - flash: NOT supported\n");
> }
>
>+ nsAutoTArray<Size, 16> sizes;
>+ mParams.Get(CAMERA_PARAM_SUPPORTED_VIDEOSIZES, sizes);
>+ if (sizes.Length() > 0) {
>+ mSeparateVideoAndPreviewSizesSupported = true;
>+ DOM_CAMERA_LOGI(" - support for separate preview and video sizes\n");
>+ mParams.Get(CAMERA_PARAM_VIDEOSIZE, mLastRecorderSize);
>+ DOM_CAMERA_LOGI(" - default video recorder size: %u x %u\n",
>+ mLastRecorderSize.width, mLastRecorderSize.height);
>+ } else {
>+ mLastRecorderSize = mCurrentConfiguration.mPreviewSize;
>+ }
>+
> return NS_OK;
> }
>
> nsGonkCameraControl::~nsGonkCameraControl()
> {
> DOM_CAMERA_LOGT("%s:%d : this=%p, mCameraHw = %p\n", __func__, __LINE__, this, mCameraHw.get());
>
> StopImpl();
>@@ -246,19 +256,16 @@ nsGonkCameraControl::SetConfigurationInt
> rv = Set(CAMERA_PARAM_RECORDINGHINT, aConfig.mMode == kVideoMode);
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to set recording hint (0x%x)\n", rv);
> }
> }
>
> mCurrentConfiguration.mMode = aConfig.mMode;
> mCurrentConfiguration.mRecorderProfile = aConfig.mRecorderProfile;
>- if (aConfig.mMode == kVideoMode) {
>- mCurrentConfiguration.mPreviewSize = mLastRecorderSize;
>- }
>
> OnConfigurationChange();
> return NS_OK;
> }
>
> nsresult
> nsGonkCameraControl::SetConfigurationImpl(const Configuration& aConfig)
> {
>@@ -311,32 +318,16 @@ nsGonkCameraControl::SetPictureConfigura
> DOM_CAMERA_LOGI("picture mode preview: wanted %ux%u, got %ux%u (%u fps)\n",
> aConfig.mPreviewSize.width, aConfig.mPreviewSize.height,
> mCurrentConfiguration.mPreviewSize.width, mCurrentConfiguration.mPreviewSize.height,
> mPreviewFps);
>
> return NS_OK;
> }
>
>-nsresult
>-nsGonkCameraControl::SetVideoConfiguration(const Configuration& aConfig)
>-{
>- DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
>-
>- nsresult rv = SetupVideoMode(aConfig.mRecorderProfile);
>- NS_ENSURE_SUCCESS(rv, rv);
>-
>- DOM_CAMERA_LOGI("video mode preview: profile '%s', got %ux%u (%u fps)\n",
>- NS_ConvertUTF16toUTF8(aConfig.mRecorderProfile).get(),
>- mLastRecorderSize.width, mLastRecorderSize.height,
>- mPreviewFps);
>-
>- return rv;
>-}
>-
> // Parameter management.
> nsresult
> nsGonkCameraControl::PushParameters()
> {
> uint32_t dcu = mDeferConfigUpdate;
> if (dcu > 0) {
> DOM_CAMERA_LOGI("Defering config update (nest level %u)\n", dcu);
> return NS_OK;
>@@ -395,22 +386,18 @@ nsGonkCameraControl::SetAndPush(uint32_t
> }
> return PushParameters();
> }
>
> // Array-of-Size parameter accessor.
> nsresult
> nsGonkCameraControl::Get(uint32_t aKey, nsTArray<Size>& aSizes)
> {
>- if (aKey == CAMERA_PARAM_SUPPORTED_VIDEOSIZES) {
>- nsresult rv = mParams.Get(aKey, aSizes);
>- if (aSizes.Length() != 0) {
>- return rv;
>- }
>- DOM_CAMERA_LOGI("Camera doesn't support video independent of the preview\n");
>+ if (aKey == CAMERA_PARAM_SUPPORTED_VIDEOSIZES &&
>+ !mSeparateVideoAndPreviewSizesSupported) {
> aKey = CAMERA_PARAM_SUPPORTED_PREVIEWSIZES;
> }
>
> return mParams.Get(aKey, aSizes);
> }
>
> // Array-of-doubles parameter accessor.
> nsresult
>@@ -1267,31 +1254,40 @@ nsGonkCameraControl::SetPreviewSize(cons
> Size best;
> rv = GetSupportedSize(aSize, previewSizes, best);
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to find a supported preview size, requested size %dx%d",
> aSize.width, aSize.height);
> return rv;
> }
>
>- // Some camera drivers will ignore our preview size if it's larger
>- // than the currently set video recording size, so we need to set
>- // the video size here as well, just in case.
>- if (best.width > mLastRecorderSize.width || best.height > mLastRecorderSize.height) {
>- SetVideoSize(best);
>+ if (mSeparateVideoAndPreviewSizesSupported) {
>+ // Some camera drivers will ignore our preview size if it's larger
>+ // than the currently set video recording size, so we need to set
>+ // the video size here as well, just in case.
>+ if (best.width > mLastRecorderSize.width || best.height > mLastRecorderSize.height) {
>+ SetVideoSize(best);
>+ }
>+ } else {
>+ mLastRecorderSize = best;
> }
> mCurrentConfiguration.mPreviewSize = best;
> return Set(CAMERA_PARAM_PREVIEWSIZE, best);
> }
>
> nsresult
> nsGonkCameraControl::SetVideoSize(const Size& aSize)
> {
> MOZ_ASSERT(NS_GetCurrentThread() == mCameraThread);
>
>+ if (!mSeparateVideoAndPreviewSizesSupported) {
>+ DOM_CAMERA_LOGE("Camera does not support setting separate video size\n");
>+ return NS_ERROR_NOT_AVAILABLE;
>+ }
>+
> nsTArray<Size> videoSizes;
> nsresult rv = Get(CAMERA_PARAM_SUPPORTED_VIDEOSIZES, videoSizes);
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Camera failed to return any video sizes (0x%x)\n", rv);
> return rv;
> }
>
> Size best;
>@@ -1363,25 +1359,24 @@ nsGonkCameraControl::GetSupportedSize(co
> rv = NS_OK;
> }
> }
> }
> return rv;
> }
>
> nsresult
>-nsGonkCameraControl::SetupVideoMode(const nsAString& aProfile)
>+nsGonkCameraControl::SetVideoConfiguration(const Configuration& aConfig)
> {
> DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
>
> // read preferences for camcorder
> mMediaProfiles = MediaProfiles::getInstance();
>
>- nsAutoCString profile = NS_ConvertUTF16toUTF8(aProfile);
>- // XXXkhuey are we leaking?
>+ nsAutoCString profile = NS_ConvertUTF16toUTF8(aConfig.mRecorderProfile);
> mRecorderProfile = GetGonkRecorderProfileManager().take()->Get(profile.get());
> if (!mRecorderProfile) {
> DOM_CAMERA_LOGE("Recorder profile '%s' is not supported\n", profile.get());
> return NS_ERROR_INVALID_ARG;
> }
>
> const GonkRecorderVideoProfile* video = mRecorderProfile->GetGonkVideoProfile();
> int width = video->GetWidth();
>@@ -1396,31 +1391,43 @@ nsGonkCameraControl::SetupVideoMode(cons
> PullParametersImpl();
>
> Size size;
> size.width = static_cast<uint32_t>(width);
> size.height = static_cast<uint32_t>(height);
>
> {
> ICameraControlParameterSetAutoEnter set(this);
>+ nsresult rv;
>
>- // The camera interface allows for hardware to provide two video
>- // streams, a low resolution preview and a potentially high resolution
>- // stream for encoding. For now we don't use this and set preview and video
>- // size to the same thing.
>- nsresult rv = SetVideoSize(size);
>- if (NS_FAILED(rv)) {
>- DOM_CAMERA_LOGE("Failed to set video mode video size (0x%x)\n", rv);
>- return rv;
>- }
>+ if (mSeparateVideoAndPreviewSizesSupported) {
>+ // The camera supports two video streams: a low(er) resolution preview
>+ // stream and and a potentially high(er) resolution stream for encoding.
>+ rv = SetVideoSize(size);
>+ if (NS_FAILED(rv)) {
>+ DOM_CAMERA_LOGE("Failed to set video mode video size (0x%x)\n", rv);
>+ return rv;
>+ }
>
>- rv = SetPreviewSize(size);
>- if (NS_FAILED(rv)) {
>- DOM_CAMERA_LOGE("Failed to set video mode preview size (0x%x)\n", rv);
>- return rv;
>+ // The video size must be set first, before the preview size, because
>+ // some platforms have a dependency between the two.
>+ rv = SetPreviewSize(aConfig.mPreviewSize);
>+ if (NS_FAILED(rv)) {
>+ DOM_CAMERA_LOGE("Failed to set video mode preview size (0x%x)\n", rv);
>+ return rv;
>+ }
>+ } else {
>+ // The camera only supports a single video stream: in this case, we set
>+ // the preview size to be the desired video recording size, and ignore
>+ // the specified preview size.
>+ rv = SetPreviewSize(size);
>+ if (NS_FAILED(rv)) {
>+ DOM_CAMERA_LOGE("Failed to set video mode preview size (0x%x)\n", rv);
>+ return rv;
>+ }
> }
>
> rv = Set(CAMERA_PARAM_PREVIEWFRAMERATE, fps);
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to set video mode frame rate (0x%x)\n", rv);
> return rv;
> }
> }
>diff --git a/dom/camera/GonkCameraControl.h b/dom/camera/GonkCameraControl.h
>--- a/dom/camera/GonkCameraControl.h
>+++ b/dom/camera/GonkCameraControl.h
>@@ -121,17 +121,16 @@ protected:
> virtual nsresult PushParametersImpl() MOZ_OVERRIDE;
> virtual nsresult PullParametersImpl() MOZ_OVERRIDE;
> virtual already_AddRefed<RecorderProfileManager> GetRecorderProfileManagerImpl() MOZ_OVERRIDE;
> already_AddRefed<GonkRecorderProfileManager> GetGonkRecorderProfileManager();
>
> nsresult SetupRecording(int aFd, int aRotation, uint64_t aMaxFileSizeBytes,
> uint64_t aMaxVideoLengthMs);
> nsresult SetupRecordingFlash(bool aAutoEnableLowLightTorch);
>- nsresult SetupVideoMode(const nsAString& aProfile);
> nsresult SetPreviewSize(const Size& aSize);
> nsresult SetVideoSize(const Size& aSize);
> nsresult PausePreview();
> nsresult GetSupportedSize(const Size& aSize, const nsTArray<Size>& supportedSizes, Size& best);
>
> friend class SetPictureSize;
> friend class SetThumbnailSize;
> nsresult SetPictureSize(const Size& aSize);
>@@ -147,16 +146,17 @@ protected:
> Size mLastPictureSize;
> Size mLastThumbnailSize;
> Size mLastRecorderSize;
> uint32_t mPreviewFps;
> bool mResumePreviewAfterTakingPicture;
> bool mFlashSupported;
> bool mLuminanceSupported;
> bool mAutoFlashModeOverridden;
>+ bool mSeparateVideoAndPreviewSizesSupported;
> Atomic<uint32_t> mDeferConfigUpdate;
> GonkCameraParameters mParams;
>
> nsRefPtr<mozilla::layers::ImageContainer> mImageContainer;
>
> android::MediaProfiles* mMediaProfiles;
> nsRefPtr<android::GonkRecorder> mRecorder;
> // Touching mRecorder happens inside this monitor because the destructor
Attachment #8489445 -
Flags: feedback?(shinuk153) → feedback+
Comment 37•10 years ago
|
||
(In reply to Mike Habicher [:mikeh] from comment #36)
> Thank you for the video. We don't see that size glitch happening on our
> reference device. What code (gecko and gaia) are you running?
gonk c4765e55870c241a5d25499d65323f3e87d26143
gecko ef26d06076dd8909d82a1c0e1c2f501bc89b5161
gaia ddec117b2d6ac8ea50d7fd833a9cf0605d5b358b
Comment 38•10 years ago
|
||
Assignee | ||
Comment 39•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Comment 41•10 years ago
|
||
Hi Mike, could you provide a 2.0M patch? Thanks.
blocking-b2g: backlog → 2.0M+
Flags: needinfo?(mhabicher)
Assignee | ||
Updated•10 years ago
|
status-b2g-v2.0M:
--- → affected
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 42•10 years ago
|
||
Rebased onto b2g32_v2_0m.
Attachment #8489718 -
Attachment is obsolete: true
Attachment #8489856 -
Attachment is obsolete: true
Attachment #8494634 -
Flags: review+
Attachment #8494634 -
Flags: approval-mozilla-b2g32?
Assignee | ||
Comment 43•10 years ago
|
||
(Not sure if a-b2g32 is required for b2g32m.)
Assignee | ||
Comment 44•10 years ago
|
||
Add "a=2.0+" to the patch description.
Attachment #8494634 -
Attachment is obsolete: true
Attachment #8494634 -
Flags: approval-mozilla-b2g32?
Attachment #8494647 -
Flags: review+
Assignee | ||
Comment 45•10 years ago
|
||
howie, can you please complete the approval-b2g32 request on attachment 8494647 [details] [diff] [review]? The patch was tested on master/2.2, and the 2.0m codebase is nearly the same.
Flags: needinfo?(hochang)
Comment 46•10 years ago
|
||
Hi Kai-Zhen, need your help to pick this patch to 2.0M, thank you.
Flags: needinfo?(hochang) → needinfo?(kli)
Comment 47•10 years ago
|
||
Flags: needinfo?(kli)
Comment 48•10 years ago
|
||
Hi Kai Zhen,
This is generic bug. Is it possible to uplift patch from 2.0M to 2.0?
Thanks!
Comment 49•10 years ago
|
||
It is able to merge the patch from 2.0m to 2.0. But to land into 2.0 need to get approval-mozilla-b2g32+. This request of approval was obsoleted in comment 45.
If 2.0 branch really need this patch, need to request the approval again.
Flags: needinfo?(kli)
Comment 50•10 years ago
|
||
Hi! Howie,
This is a generic bug.
Could you help to get approval-mozilla-b2g32+?
Thanks
--
Keven
(In reply to Kai-Zhen Li [:seinlin] from comment #50)
> It is able to merge the patch from 2.0m to 2.0. But to land into 2.0 need to
> get approval-mozilla-b2g32+. This request of approval was obsoleted in
> comment 45.
>
> If 2.0 branch really need this patch, need to request the approval again.
Flags: needinfo?(hochang)
Comment 51•10 years ago
|
||
Thanks Keven. Although it's a generic bug, based on Comment 24 and Comment 25, changing to 2.0? since not sure this is critical enough to block 2.0 at this moment.
blocking-b2g: 2.0+ → 2.0?
Flags: needinfo?(hochang)
Comment 52•10 years ago
|
||
Does this need to land on v2.1? If so, please nominate it for Aurora approval :)
status-b2g-v2.1:
--- → ?
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → ?
status-firefox35:
--- → fixed
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 53•10 years ago
|
||
Comment on attachment 8489445 [details] [diff] [review]
Uncouple preview and video sizes, v2
Approval Request Comment
[Feature/regressing bug #]: bug 1014877
[User impact if declined]: preview size and recorded video size must be the same
[Describe test coverage new/current, TBPL]: manual testing on Flame and Nexus 4; emulator limitations preclude TBPL automated tests at this time
[Risks and why]: may trigger previously-unexercised and flaky code-paths on other platforms
[String/UUID change made/needed]: none
Attachment #8489445 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(mhabicher)
Comment 54•10 years ago
|
||
Hi Mike,Could you please verify this could be reproduce in Flame FF2.0? Thanks!
Flags: needinfo?(mlien)
Comment 55•10 years ago
|
||
Comment on attachment 8489445 [details] [diff] [review]
Uncouple preview and video sizes, v2
Approving on aurora.
Attachment #8489445 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 56•10 years ago
|
||
(In reply to Josh Cheng from comment #55)
> Hi Mike,Could you please verify this could be reproduce in Flame FF2.0?
> Thanks!
the risk highlighted in comment #54 does not see to be low, so unless this is very easily reproducible on Flame or a partner is stop-shipping for this bug as the flickering/glitch may be prominent on the device, we should not land this on 2.0
Comment 57•10 years ago
|
||
Updated•10 years ago
|
Flags: needinfo?(mlien)
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0M+
Comment 58•10 years ago
|
||
Hi Mike,
I would tried 2.0M on woodduck, and it seems there is still a slight shake when switching between picture and video mode. Would you suggest opening a new bug to discuss or catch more info in this bug? Thanks.
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 59•10 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #59)
> I would tried 2.0M on woodduck, and it seems there is still a slight shake
> when switching between picture and video mode. Would you suggest opening a
> new bug to discuss or catch more info in this bug? Thanks.
Bug 998019 is still open on this issue. Hopefully a fix for that issue fixes Woodduck as well.
Flags: needinfo?(mhabicher)
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•