985481 - [Camera][Gecko] Make sure that a supported video size is set

Status: RESOLVED FIXED

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

Description

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.patch

Previous to this change, when setting video size it was set without checking if it was a supported size. Since the video size is also set when setting the preview size, this could lead to that SetPreviewSize returned an error, which in our case made the viewfinder to stop working.

With this change whenever the video size is set we first fetch the supported sizes and find the closest size to the requested one. Since we need to do the same for both video and preview size the code that matches requested size with supported sizes has been broken out to a separate method.

Comment 1

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

Comment on attachment 8393515 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.patch

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

r-, but I think this will be good with the requested changes. Also, please see the following before posting an updated patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

::: dom/camera/GonkCameraControl.cpp
@@ +1043,5 @@
> +    DOM_CAMERA_LOGE("Failed to find a supported video size, requested size %dx%d",
> +        aSize.width, aSize.height);
> +    return rv;
> +  }
> +  return SetAndPush(CAMERA_PARAM_VIDEOSIZE, best);

If this is only called from SetPreviewSize(), please change this to Set(). The SetAndPush() call in SetPreviewSize() will then push both parameter changes to the camera.

@@ +1048,5 @@
> +}
> +
> +nsresult
> +nsGonkCameraControl::GetSupportedSize(const Size& aSize,
> +    const nsTArray<Size>& supportedSizes, Size& best)

Line up the second line of arguments with 'const' in the first line:

nsGonkCameraControl::GetSupportedSize(const Size& aSize,
		                      const nsTArray<Size>& supportedSizes,
                                      Size& best)

::: dom/camera/GonkCameraControl.h
@@ +158,5 @@
>  private:
>    nsGonkCameraControl(const nsGonkCameraControl&) MOZ_DELETE;
>    nsGonkCameraControl& operator=(const nsGonkCameraControl&) MOZ_DELETE;
> +  nsresult GetSupportedSize(const Size& aSize,
> +      const nsTArray<Size>& supportedSizes, Size& best);

Please make this protected, not private.

Comment 2

[Fredrik Lanker [:lanker]]

(In reply to Mike Habicher [:mikeh] from comment #1)
> Comment on attachment 8393515 [details] [diff] [review]
> 0001-Make-sure-that-a-supported-video-size-is-set.patch
> 
> Review of attachment 8393515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-, but I think this will be good with the requested changes. Also, please
> see the following before posting an updated patch:
> 
> https://developer.mozilla.org/en-US/docs/
> Creating_a_patch_that_can_be_checked_in

Thank for the review and the link! I'll use the git-patch-to-hg-patch script for my next upload, hope that will be better.

> ::: dom/camera/GonkCameraControl.cpp
> @@ +1043,5 @@
> > +    DOM_CAMERA_LOGE("Failed to find a supported video size, requested size %dx%d",
> > +        aSize.width, aSize.height);
> > +    return rv;
> > +  }
> > +  return SetAndPush(CAMERA_PARAM_VIDEOSIZE, best);
> 
> If this is only called from SetPreviewSize(), please change this to Set().
> The SetAndPush() call in SetPreviewSize() will then push both parameter
> changes to the camera.

It's also used in SetupVideoMode(), directly after a call to SetPreviewSize(). I guess I could switch order for those calls and make the change that you suggested, if you prefer?

Comment 3

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

(In reply to Fredrik Lanker [:lanker] from comment #2)
> (In reply to Mike Habicher [:mikeh] from comment #1)
> 
> > ::: dom/camera/GonkCameraControl.cpp
> > @@ +1043,5 @@
> > > +    DOM_CAMERA_LOGE("Failed to find a supported video size, requested size %dx%d",
> > > +        aSize.width, aSize.height);
> > > +    return rv;
> > > +  }
> > > +  return SetAndPush(CAMERA_PARAM_VIDEOSIZE, best);
> > 
> > If this is only called from SetPreviewSize(), please change this to Set().
> > The SetAndPush() call in SetPreviewSize() will then push both parameter
> > changes to the camera.
> 
> It's also used in SetupVideoMode(), directly after a call to
> SetPreviewSize(). I guess I could switch order for those calls and make the
> change that you suggested, if you prefer?

Hmm, looking at your patch again, I see the following callstack:

SetupVideoMode()
  SetPreviewSize()
    SetVideoSize()
      SetAndPush(VIDEOSIZE)
    SetAndPush(PREVIEWSIZE)
  SetVideoSize()
    SetAndPush(VIDEOSIZE)

That's definitely redundant. Does your camera support video and preview streams with different resolutions? If not, you can probably get away with removing the SetVideoSize() call from SetupVideoMode().

Comment 4

[Dave Hylands [:dhylands]]

Comment on attachment 8393515 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.patch

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

I'll say r=me if you address Mike's issues.

::: dom/camera/GonkCameraControl.cpp
@@ +1063,4 @@
>    } else if (aSize.width && aSize.height) {
>      // both height and width specified, find the supported size closest to requested size
>      uint32_t targetArea = aSize.width * aSize.height;
> +    for (uint32_t i = 0; i < supportedSizes.Length(); i++) {

nit: There are nsTArray<Size>::index_type and nsTArray<Size>::size_type types which will ensure that i has the correct type (rather than hard-coding uint32_t)

I normally do something like:

typedef nsTArray<Size> SizeArray;

And then use SizeArray to delcare my arrays and SizeArray::index_type and SizeArray::size_type

Comment 5

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.patch

Thank you Mike for noticing the less than optimal call stack. I ended up making some more changes, which I think will address that. Changelog since the last patch:

- SetVideoSize and SetPreviewSize don't do a push anymore, handled outside of the functions
- The SetVideoSize call in SetPreviewSize is only called if needed (pretty untested, I have no device where this is a problem)
- Changed order of the Set*Size calls in SetVideoMode, so that SetPreviewSize can override the video size if needed.
- Changed the fps setting in SetVideoMode so that it's pushed along with the other settings
- uint32_t -> nsTArray<Size>::index_type
- argument intendation and private -> protected according to Mike's previous review

And hopefully the patch is in a format mercurial can handle.

I think that should take care of the redundant calls. Worst case should be that SetVideoSize is called twice in SetVideoMode, but only if the preview is larger than the video, which I think would be fine.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 8394124 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.patch

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

Looks good to me.

Just a note for the future patch ettiquite. When creating a second (or third, etc.) revision of the patch, please keep the description identical except for adding a v2, v3 etc (this makes finding the previous version for interdiff easier).
Also, since I'm talking about it, if you have multi-part patches, also make sure the description has pt1 pt2 (or similar) for each part.

For this particular patch, there was only 1 previous version so it wasn't really a problem, but with larger bugs these little details start to make a difference for the reviewers.

Comment 7

[Fredrik Lanker [:lanker]]

Thanks for the approval and comment, we appreciate all advices!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3e7c91f7f31e

Comment 9

[Ed Morley [:emorley]]

Unfortunately I had to revert this due to test failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36886781&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36884828&tree=B2g-Inbound

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ef73445154cf

Comment 10

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.v2.patch

The failing test seems to be caused by not checking the return value of PushParameters in SetPictureConfiguration. In v2 it's now checked and an error is returned if it  fails.

Comment 11

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

Comment on attachment 8399973 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.v2.patch

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

Drive-by review: a few remaining suggestions.

::: dom/camera/GonkCameraControl.cpp
@@ +246,5 @@
>    nsresult rv = SetPreviewSize(aConfig.mPreviewSize);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = PushParameters();
> +  NS_ENSURE_SUCCESS(rv, rv);

Fredrik, new usage of NS_ENSURE_SUCCESS() was recently deprecated, so although it appears elsewhere in the code, the new preferred style is:

  if (NS_FAILED(rv)) {
    return rv;
  }

Or, if you want to log the error (and we probably do here):

  if (NS_WARN_IF(NS_FAILED(rv)) {
    return rv;
  }

@@ +1022,5 @@
> +
> +  // 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 * best.height > mLastRecorderSize.width * mLastRecorderSize.height) {

I think this condition should be:

  if (best.width > mLastRecorderSize.width || best.height > mLastRecorderSize.height) {

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8399973 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.v2.patch

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

nit: NS_ENSURE_SUCCESS is deprecated.See bug 672843 and
https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ

Comment 13

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.v3.patch

- removed the use of NS_ENSURE_SUCCESS
- changed the preview and video size check

(btw, https://developer.mozilla.org/en/docs/NS_ENSURE_SUCCESS should probably be updated)

Comment 14

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

Comment on attachment 8400480 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.v3.patch

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

Thanks, Fredrik! Last nits.

::: dom/camera/GonkCameraControl.cpp
@@ +244,5 @@
>    mRecorderProfile = nullptr;
>  
>    nsresult rv = SetPreviewSize(aConfig.mPreviewSize);
> +  if (NS_WARN_IF(NS_FAILED(rv)))
> +      return rv;

Please use braces, even on single-lines of code. Also, the indentation should only be two spaces.

@@ +248,5 @@
> +      return rv;
> +
> +  rv = PushParameters();
> +  if (NS_WARN_IF(NS_FAILED(rv)))
> +      return rv;

Same here.

Comment 15

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.v4.patch

- added braces (I was following the style of Benjamin's patches in bug 672843, but I should've looked at the coding style guidelines [1] instead...)
- fixed indentation, sorry about that

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Use_the_nice_macros

Comment 16

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

Comment on attachment 8401139 [details] [diff] [review]
0001-Make-sure-that-a-supported-video-size-is-set.v4.patch

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

Looks good. Thanks for the patch and for working through all the gotchas!

Comment 17

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

Actually, before tagging this as 'checkin-needed', please attach a version of the patch with 8 lines of pre- and post patch context, as per:

https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

In hg, this is done by specified 'hg diff -U8' or 'hg qdiff -U8'; in git, it is 'git diff -U8'.

Also, the description doesn't need "[Camera][Gecko]" in it, and should have the reviewer; e.g. "Bug 985481 - make sure a supported video size it set, r=mikeh".

Comment 18

[Fredrik Lanker [:lanker]]

Attachment: 0001-Make-sure-that-a-supported-video-size-is-set.v5.patch

Thank you Mike for the reviews!

- updated the commit message and added more context to the diff.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1832c86557f6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1832c86557f6

Comment 21

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

Blocks a 1.4 blocker.

Comment 22

[Hema Koka [:hema]]

This fix is required for a critical blocker 1000567

Comment 23

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

I've confirmed that the patch above lands cleanly onto b2g30_v1_4.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c6a0838c1585