Closed Bug 1014877 Opened 6 years ago Closed 5 years ago

[Camera][Gecko] Uncouple preview and video frame size dependencies

Categories

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

ARM
Gonk (Firefox OS)
enhancement
Not set

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)

RESOLVED FIXED
2.1 S5 (26sep)
blocking-b2g 2.0M+
Tracking Status
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)

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.
Attached patch SupportedSize_patch (obsolete) — Splinter Review
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 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?
shinuk - Before considering this for 2.0 blocking, can you please comment on the user impact of this bug?
Flags: needinfo?(shinuk153)
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?
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? → ---
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)
(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)
Attached patch Uncouple_preview_video_size (obsolete) — Splinter Review
Attachment #8463823 - Attachment is obsolete: true
Attachment #8463823 - Flags: review?(mhabicher)
Attachment #8468150 - Flags: review?(mhabicher)
Flags: needinfo?(lmandel)
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-
Moving this to 2.1 release milestone.
Flags: needinfo?(hkoka)
Target Milestone: --- → 2.1 S3 (29aug)
Flags: needinfo?(mhabicher)
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)
> 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)
Duplicate of this bug: 1057276
[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?
Flags: needinfo?(vliu)
(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
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)
(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.
(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)
Target Milestone: 2.1 S3 (29aug) → ---
(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)
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)
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
Whiteboard: [priority]
(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: nobody → mhabicher
Flags: needinfo?(mhabicher)
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 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+
(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)
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 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-
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)
Attachment #8489445 - Flags: review?(aosmond) → review+
Patch tested on Flame, works properly.

Try-server push: https://tbpl.mozilla.org/?tree=Try&rev=97cf44dbba8b
Attached video video showing flickering (obsolete) —
Flags: needinfo?(shinuk153)
(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.
(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 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+
(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
Attached file log flickering (obsolete) —
https://hg.mozilla.org/mozilla-central/rev/6b7ab2f8fc1f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Blocks: 1057276
Blocks: 1070799
Hi Mike, could you provide a 2.0M patch? Thanks.
blocking-b2g: backlog → 2.0M+
Flags: needinfo?(mhabicher)
Flags: needinfo?(mhabicher)
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?
(Not sure if a-b2g32 is required for b2g32m.)
Add "a=2.0+" to the patch description.
Attachment #8494634 - Attachment is obsolete: true
Attachment #8494634 - Flags: approval-mozilla-b2g32?
Attachment #8494647 - Flags: review+
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)
Hi Kai-Zhen, need your help to pick this patch to 2.0M, thank you.
Flags: needinfo?(hochang) → needinfo?(kli)
Hi Kai Zhen,
This is generic bug. Is it possible to uplift patch from 2.0M to 2.0?
Thanks!
blocking-b2g: 2.0M+ → 2.0+
Flags: needinfo?(kli)
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)
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)
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)
Does this need to land on v2.1? If so, please nominate it for Aurora approval :)
status-b2g-v2.1: --- → ?
Flags: needinfo?(mhabicher)
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)
Hi Mike,Could you please verify this could be reproduce in Flame FF2.0? Thanks!
Flags: needinfo?(mlien)
Blocks: Woodduck
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+
(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
Flags: needinfo?(mlien)
blocking-b2g: 2.0? → 2.0M+
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)
(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)
Blocks: 1080481
See Also: → 998019
Depends on: 1078435
Depends on: 1098028
You need to log in before you can comment on or make changes to this bug.