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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

[shinuk]

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.

Comment 2

[shinuk]

Attachment: SupportedSize_patch

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:

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 4

[shinuk]

[Blocking Requested - why for this release]:
GonkCameraControl doesn't map exact preview size and need to separate recording video size and preview frame size.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

shinuk - Before considering this for 2.0 blocking, can you please comment on the user impact of this bug?

Comment 6

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

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

[Hema Koka [:hema]]

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

Comment 8

[shinuk]

(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.

Comment 9

[shinuk]

(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.

Comment 10

[shinuk]

(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.

Comment 11

[shinuk]

Attachment: Uncouple_preview_video_size

Comment 12

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

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.

Comment 13

[Hema Koka [:hema]]

Moving this to 2.1 release milestone.

Comment 14

[Hema Koka [:hema]]

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

Comment 15

[shinuk]

> 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.

Comment 17

[Vincent Liu[:vliu]]

[Blocking Requested - why for this release]:

Partner device runs on v2.0 branch needs this patch to fix the problem.

Comment 19

[Hema Koka [:hema]]

(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 20

[Vincent Liu[:vliu]]

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?".

Comment 21

[Vincent Liu[: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.

Comment 22

[Hema Koka [:hema]]

(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?

Comment 23

[Vincent Liu[:vliu]]

(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.

Comment 24

[Jason Smith [:jsmith]]

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?

Comment 25

[Hema Koka [:hema]]

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

Comment 26

[Vincent Liu[:vliu]]

(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.

Comment 27

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

Attachment: Uncouple preview and video sizes, v1

Shinuk, try this out. It uncouples the preview size and the video recording size.

Comment 28

[shinuk]

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.

Comment 29

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

(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.

Comment 30

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

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

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 32

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

Attachment: Uncouple preview and video sizes, v2

Incorporate review feedback.

Shinuk, can you please test this patch as well? It changes a few things.

Comment 33

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

Patch tested on Flame, works properly.

Try-server push: https://tbpl.mozilla.org/?tree=Try&rev=97cf44dbba8b

Comment 34

[shinuk]

Attachment: video showing flickering

Comment 35

[shinuk]

(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.

Comment 36

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

(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?

Comment 37

[shinuk]

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

Comment 38

[shinuk]

(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 39

[shinuk]

Attachment: log flickering

Comment 40

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

https://hg.mozilla.org/integration/b2g-inbound/rev/6b7ab2f8fc1f

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b7ab2f8fc1f

Comment 42

[howie [:howie]]

Hi Mike, could you provide a 2.0M patch? Thanks.

Comment 43

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

Attachment: Uncouple preview and video sizes, v2 r=aosmond [v2.0m]

Rebased onto b2g32_v2_0m.

Comment 44

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

(Not sure if a-b2g32 is required for b2g32m.)

Comment 45

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

Attachment: Uncouple preview and video sizes, v2.0.1 r=aosmond [v2.0m]

Add "a=2.0+" to the patch description.

Comment 46

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

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.

Comment 47

[howie [:howie]]

Hi Kai-Zhen, need your help to pick this patch to 2.0M, thank you.

Comment 48

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/4298deede205

Comment 49

[Josh Cheng [:josh]]

Hi Kai Zhen,
This is generic bug. Is it possible to uplift patch from 2.0M to 2.0?
Thanks!

Comment 50

[Kai-Zhen Li [:seinlin][: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.

Comment 51

[Keven Kuo [:kkuo]]

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.

Comment 52

[howie [:howie]]

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.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on v2.1? If so, please nominate it for Aurora approval :)

Comment 54

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

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

Comment 55

[Josh Cheng [:josh]]

Hi Mike,Could you please verify this could be reproduce in Flame FF2.0? Thanks!

Comment 56

[bhavana bajaj [:bajaj]]

Comment on attachment 8489445 [details] [diff] [review]
Uncouple preview and video sizes, v2

Approving on aurora.

Comment 57

[bhavana bajaj [:bajaj]]

(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 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f5170efd1061

Comment 59

[Vincent Liu[:vliu]]

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.

Comment 60

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

(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.