[Camera][Gecko] Ensure that we can't set a preview size > video size in video mode

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.1 affected)

Details

Attachments

(2 attachments, 6 obsolete attachments)

22.16 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
23.09 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Fixing this issue should alleviate bug 1098028.
Created attachment 8522609 [details] [diff] [review]
WIP - make sure preview size <= video size, v1

This is what I'm thinking. I need to check a few more things before asking for a formal r?.
Attachment #8522609 - Flags: feedback?(aosmond)
Comment on attachment 8522609 [details] [diff] [review]
WIP - make sure preview size <= video size, v1

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

1) Do we have a similar problem with picture size and thumbnail size vs preview size?

2) GonkCameraSource::configureCamera can set preview size and video size. Could this interfere in any way?

3) GonkCameraControl::GetSupportedSize needs to be updated to understand that the "best" selection has additional limitations. Since the supported sizes may be different between preview and video, it may end up inadvertently selecting a bad size that is, for example, the closest in area (but we need smaller or bigger than a particular size as the case may be!).

4) This is a little more on the art side of things, but: I dislike mode being passed into SetPreviewSize and SetVideoSize. I dislike SetPreviewSize and SetVideoSize having dependencies referencing back to each other; it would be very easy to mess up the order and break things some of the time... perhaps there should be a function which we should use for setting both of them (cannot/should not set them individually), which also determines the best *pairing* for both of them.

::: dom/camera/GonkCameraControl.cpp
@@ +1321,5 @@
> +         best.height < mCurrentConfiguration.mPreviewSize.height)) {
> +      SetPreviewSize(best, aMode);
> +    }
> +  } else {
> +    mCurrentConfiguration.mPreviewSize = best;

SetVideoSize now happens after SetPreviewSize, so what is configured in the parameters and what is cached in our layer may mismatch in the else case? Even if it happened first, you would need SetPreviewSize to look at mCurrentConfiguration instead of aConfig.
Attachment #8522609 - Flags: feedback?(aosmond) → feedback+
Created attachment 8522710 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1 -- IGNORE, WRONG PATCH!

Thanks for the feedback, Andrew. I decided to take this in a pretty different direction, so it's really v1 of a new patch.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5c6afe14d971
Attachment #8522609 - Attachment is obsolete: true
Attachment #8522710 - Flags: review?(aosmond)
Created attachment 8522711 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1

Let's attach the RIGHT patch this time, shall we?
Attachment #8522710 - Attachment is obsolete: true
Attachment #8522710 - Flags: review?(aosmond)
Attachment #8522711 - Flags: review?(aosmond)
Attachment #8522710 - Attachment description: Fix the (complex) requirements for video mode preview size, v1 → Fix the (complex) requirements for video mode preview size, v1 -- IGNORE, WRONG PATCH!
Created attachment 8522715 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.1

Forgot to set mCurrentConfiguration.mPreviewSize when !mSeparateVideoAndPreviewSizesSupported.
Attachment #8522711 - Attachment is obsolete: true
Attachment #8522711 - Flags: review?(aosmond)
Attachment #8522715 - Flags: review?(aosmond)
FYI, justindarc: with this patch applied, the video-mode viewfinder will be 720x480 instead of 864x480, so it will no longer fill the screen. Unfortunately this is a limitation of the driver/API.
(In reply to Andrew Osmond [:aosmond] from comment #2)

> 1) Do we have a similar problem with picture size and thumbnail size vs
> preview size?

Some drivers do, and we handle that in SetThumbnailSizeImpl().

> 2) GonkCameraSource::configureCamera can set preview size and video size.
> Could this interfere in any way?

It doesn't look like those values are used for anything other than configuring the camera, which we've already done. A simple change for safety would be to have GonkRecorder just pass in -1x-1 for those values. Unfortunately GonkRecorder needs the frame size to configure the MPEG4 writer.
Comment on attachment 8522715 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.1

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

::: dom/camera/GonkCameraControl.cpp
@@ +354,5 @@
> +        mLastRecorderSize = preview;
> +      } else {
> +        DOM_CAMERA_LOGW("Failed to increase video size to preview size (0x%x)\n", rv);
> +      }
> +    }

I still think we should consider setting VIDEOSIZE to always match PREVIEWSIZE in picture mode. Leaves less to the imagination of the driver and more consistent in our behaviour. Switching modes between picture and video will probably result in these values changing anyways due to the differing selection algorithms.

@@ +1302,5 @@
>      return NS_OK;
>    } else if (aSize.width && aSize.height) {
>      // both height and width specified, find the supported size closest to
>      // the requested size, looking for an exact match first
> +    for (SizeIndex i = 0; i < aSupportedSizes.Length(); i++) {

Nit: Don't we normally use preincrement instead of postincrement unless the latter is needed?

@@ +1419,5 @@
> +  Size preferred;
> +  rv = Get(CAMERA_PARAM_PREFERRED_PREVIEWSIZE_FOR_VIDEO, preferred);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

Does preferred preview size ever change based on the presently configured video size? (In which case we would see different results if there was a different earlier configuration, such as would be possible if the recorder profile changed or the picture mode altered the video size...?)

@@ +1453,5 @@
> +    if (area > preferredArea) {
> +      continue;
> +    }
> +
> +    const uint32_t delta = area - preferredArea;

We know that area <= preferredArea, thus delta would always result in 0 or a negative number if signed math were to be used. Since we are using unsigned math, then we should be *maximizing* delta (unless it is zero) instead of minimizing? Or just switch to signed math :).

@@ +1454,5 @@
> +      continue;
> +    }
> +
> +    const uint32_t delta = area - preferredArea;
> +    if (s.width * video.height / s.height == video.width) {

Isn't s.width * video.height == video.width * s.height is the same but doesn't have any rounding implications?
(In reply to Andrew Osmond [:aosmond] from comment #8)
> @@ +1453,5 @@
> > +    if (area > preferredArea) {
> > +      continue;
> > +    }
> > +
> > +    const uint32_t delta = area - preferredArea;
> 
> We know that area <= preferredArea, thus delta would always result in 0 or a
> negative number if signed math were to be used. Since we are using unsigned
> math, then we should be *maximizing* delta (unless it is zero) instead of
> minimizing? Or just switch to signed math :).

Err I mean, use preferredArea - area. I started this review before breakfast, I blame that :).
(In reply to Andrew Osmond [:aosmond] from comment #8)

> I still think we should consider setting VIDEOSIZE to always match
> PREVIEWSIZE in picture mode. Leaves less to the imagination of the driver
> and more consistent in our behaviour. Switching modes between picture and
> video will probably result in these values changing anyways due to the
> differing selection algorithms.

You're probably right. I initially avoided doing this so as to not preclude some kind of magical future dual-mode camera that could record video and take pictures at the same time ... but at this point there are probably enough driver corner cases to preclude that.

> @@ +1419,5 @@
> > +  Size preferred;
> > +  rv = Get(CAMERA_PARAM_PREFERRED_PREVIEWSIZE_FOR_VIDEO, preferred);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return rv;
> > +  }
> 
> Does preferred preview size ever change based on the presently configured
> video size? (In which case we would see different results if there was a
> different earlier configuration, such as would be possible if the recorder
> profile changed or the picture mode altered the video size...?)

It looks like the Flame library just uses the first video size as the preferred preview size, sets it once on initialization, and never touches it again.

(In reply to Andrew Osmond [:aosmond] from comment #9)

> Err I mean, use preferredArea - area. I started this review before
> breakfast, I blame that :).

I blame that mistake on writing the patch after midnight, while wet, and being exposed to bright lights.
Created attachment 8523042 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.2

Incorporates review feedback and cleans up some variable type issues. Also adds a short-circuit path on delta == 0 when aspect ratio matches.
Attachment #8522715 - Attachment is obsolete: true
Attachment #8522715 - Flags: review?(aosmond)
Attachment #8523042 - Flags: review?(aosmond)
Comment on attachment 8523042 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.2

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

r+ conditional on fixing that below.

::: dom/camera/GonkCameraControl.cpp
@@ +348,5 @@
> +    // than the currently set video recording size, so in picture mode, we
> +    // give preview size priority, and bump up the video size just in case.
> +    if (preview.width > mLastRecorderSize.width ||
> +        preview.height > mLastRecorderSize.height) {
> +      rv = Set(CAMERA_PARAM_VIDEOSIZE, preview);

Hm, Flame supports more preview sizes than video sizes so they don't overlap 100%.
Attachment #8523042 - Flags: review?(aosmond) → review+
Created attachment 8523113 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.3

The test at GonkCameraControl.cpp:329 always seems to be true, even when preview = 864x480 and video = 720x480.
Attachment #8523042 - Attachment is obsolete: true
Attachment #8523113 - Flags: feedback?(aosmond)
Comment on attachment 8523113 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.3

This works. r? because it's a sizeable change.
Attachment #8523113 - Flags: feedback?(aosmond) → review?(aosmond)
Comment on attachment 8523113 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.3

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

Conditional r+ with comments addressed.

::: dom/camera/GonkCameraControl.cpp
@@ +1366,5 @@
>      }
>  
> +    // no exact match on dimensions--look for a match closest in area
> +    const uint32_t targetArea = aSize.width * aSize.height;
> +    for (SizeIndex i = 0; i < aSupportedSizes.Length(); i++) {

Nit: ++i

@@ +1378,5 @@
>        }
>      }
>    } else if (!aSize.width) {
>      // width not specified, find closest height match
> +    for (SizeIndex i = 0; i < aSupportedSizes.Length(); i++) {

Nit: ++i

@@ +1389,5 @@
>        }
>      }
>    } else if (!aSize.height) {
>      // height not specified, find closest width match
> +    for (SizeIndex i = 0; i < aSupportedSizes.Length(); i++) {

Nit: ++i

@@ +1479,5 @@
> +  // If the requested preview size has the same aspect ratio as the
> +  // requested video size, *and* is the same size or smaller than
> +  // the preferred video size, then we're done.
> +  const uint32_t preferredArea = preferred.width * preferred.height;
> +  if (video.width * aConfig.mPreviewSize.height == aConfig.mPreviewSize.width * video.height &&

Shouldn't this be preview instead of aConfig.mPreviewSize?

@@ +1503,5 @@
> +    const Size& s = sizes[i];
> +    const uint32_t area = s.width * s.height;
> +    if (area > preferredArea) {
> +      continue;
> +    }

I find this an interesting contrast with line 350 in MaybeAdjustVideoSize. Here we compare the areas where as in MaybeAdjustVideoSize we compare the width/height. The latter is a stronger condition, as s.width > preferred.width && s.height > preferred.height => area > preferredArea, where as area > preferredArea => s.width > preferred.width || s.height > preferred.height.

My instinct is that we would want the stronger condition, but the aspect ratio check probably saves us...? My brain just tripped up on the idea we are calculating very similar things, but did it slightly differently for reasons I do not grasp.
Attachment #8523113 - Flags: review?(aosmond) → review+
(In reply to Andrew Osmond [:aosmond] from comment #15)

> @@ +1503,5 @@
> > +    const Size& s = sizes[i];
> > +    const uint32_t area = s.width * s.height;
> > +    if (area > preferredArea) {
> > +      continue;
> > +    }
> 
> I find this an interesting contrast with line 350 in MaybeAdjustVideoSize.
> Here we compare the areas where as in MaybeAdjustVideoSize we compare the
> width/height. The latter is a stronger condition, as s.width >
> preferred.width && s.height > preferred.height => area > preferredArea,
> where as area > preferredArea => s.width > preferred.width || s.height >
> preferred.height.

I agree it's a weaker test, but it's based _entirely_ on the description here:

http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/hardware/Camera.java#2386

...which says that "...the product of the width and height of the preview size should not be larger than that of the preferred preview size."
Created attachment 8523212 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.4 r=aosmond

Incorporate review feedback, carrying r+ forward.

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0393bac7c911
Attachment #8523113 - Attachment is obsolete: true
Attachment #8523212 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ee0ef288a8da
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
See Also: → bug 1100110
Blocks: 1078435
Depends on: 1054803
[Blocking Requested - why for this release]: This change in combination with bug 1054803 resolves the issue in bug 1078435 (which is blocking for 2.1 already).

Videos may be recorded with static/corrupt portions in the upper quarter of the display preview and saved recording. This happens when one uses the software home button and has been reported on other devices; it depends on the preview resolution and recording profiles available on the device and this change fixes the selection algorithms to ensure a compatible pairs are used.
blocking-b2g: --- → 2.1?
status-b2g-v2.1: --- → affected
Blocks: 1104739
Created attachment 8528389 [details] [diff] [review]
Fix the (complex) requirements for video mode preview size, v1.4 r=aosmond -- for b2g34 / v2.1

Updated for 2.1 given dependent changes we are unlikely to promote are missing.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3d291b84151
Attachment #8528389 - Flags: review+

Updated

4 years ago
blocking-b2g: 2.1? → 2.1+
Blocks: 1104913
Note that this no longer is desired to resolve bug 1078435. We will solve it in gaia in a much simpler fashion.
blocking-b2g: 2.1+ → ---
Bhavana, this is now denom'ed, and bug 1098028 was dependent on it.  Are we tracking another bug to have that issue fixed?  Andrew, could you point us to the new bug that fixes this issue?
Flags: needinfo?(bbajaj)
Flags: needinfo?(aosmond)
njpark: Bug 1078435 has a patch for v2.1 which forces the recording preview to be the same aspect ratio as the recorded video. The selection logic is being moved into gecko for master, but remains in gaia for 2.1 and earlier, so it made more sense to do a gaia patch instead.

It it doesn't fix bug 1098028 as well, let me know. I expect it will.
Flags: needinfo?(aosmond)
aosmond: thanks!, I marked bug 1098028 so once Bug 1078435 is fixed, we'll test that out as well.
Flags: needinfo?(bbajaj)
You need to log in before you can comment on or make changes to this bug.