804359 - Camera - video recording - expose runtime-detected platform-specific profiles

Status: RESOLVED FIXED

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

Description

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

We build using AOSP headers, but platforms may support additional recording profiles beyond those defined in media/MediaProfiles.h.  We need to be able to detect these and add them to the profiles exposed by CameraCapabilities.recorderProfiles.

Comment 1

[Andrew Overholt [:overholt]]

Mike, are we still going to have multiple profiles?

Comment 2

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

Not for v1.

Comment 3

[Inder]

Reviving it for v1.3.
We should do it for jb builds. We are unnecessarily restricting the video resolutions (cif) for JB builds which can certainly support WVGA+ resolutions.

Comment 4

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

1.3 is FC as of last Monday, so this will have to wait for 1.4.

Comment 5

[Inder]

:mikeh -- Looks like bug 896425 will give us what we want so clearing the block.

Comment 6

[Preeti Raghunath(:Preeti)]

Hema

Moved the category to Camera and requesting your review on the same.

Comment 7

[Hema Koka [:hema]]

we will keep it under our radar - but currently not in the plan for 1.4

Comment 8

[Jason Smith [:jsmith]]

This is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 9

[Hema Koka [:hema]]

This is not going into 2.0 at this point. Only critical blockers and blockers on committed features a couple of weeks before 2.0 FC

Comment 10

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

Attachment: Detect profiles at runtime, v1

This patch works around the fact that some headers are missing enum values by matching the profiles by configured resolution.

Profiles discovered:
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 0 'low' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 1 'high' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 2 'qcif' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 3 'cif' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 4 '480p' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 5 '720p' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 6 not supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 7 'qvga' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 8 'fwvga' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 9 'wvga' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 10 'vga' supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 11 not supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 12 not supported by platform
11-26 19:37:24.240 14123 14229 I Gecko   : Profile 13 not supported by platform

vga, fwvga, wvga and qvga are the newly detected profiles on flame.

Comment 11

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

Attachment: Detect profiles at runtime, v2

Make some minor tweaks.

Comment 12

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

Comment on attachment 8529405 [details] [diff] [review]
Detect profiles at runtime, v2

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

::: dom/camera/GonkRecorderProfiles.cpp
@@ +274,1 @@
>        if (IsProfileSupported(aCameraId, i)) {

Save the tabs! Invert logic here and use 'continue'.

  if (!IsProfileSupported(...
    DOM_CAMERA_LOGI(...
    continue;
  }

  // etc.

@@ +287,5 @@
> +
> +           In case of collisions, we prefer the entries with a known enum
> +           key over those without. */
> +
> +        int bestMatchIndex = -1;

Init using the constant from above.

@@ +299,5 @@
> +            bestMatchIndex = k;
> +          }
> +        }
> +
> +        if (bestMatchIndex == -1) {

Test using the constant above.

::: dom/camera/GonkRecorderProfiles.def
@@ +32,4 @@
>   *
>   * See bug 804359.
>   */
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_INVALID, "fwvga", 864, 480)

Maybe _UNKNOWN instead of _INVALID.

Comment 13

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

Attachment: Detect profiles at runtime, v3

Update based on review and IRC discussion.

Comment 14

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

Attachment: Detect profiles at runtime, v3.1

Fix comments and whitespace.

Comment 15

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

Comment on attachment 8529833 [details] [diff] [review]
Detect profiles at runtime, v3

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

::: dom/camera/GonkRecorderProfiles.cpp
@@ +35,3 @@
>  };
>  
> +const static size_t ProfileListSize = sizeof(ProfileList)/sizeof(ProfileList[0]);

... = MOZ_ARRAY_LENGTH(ProfileList);

@@ +44,5 @@
> +} ProfileListDetect[] = {
> +  #include "GonkRecorderProfiles.def"
> +};
> +
> +const static size_t ProfileListDetectSize = sizeof(ProfileListDetect)/sizeof(ProfileListDetect[0]);

Ditto.

@@ +323,5 @@
> +       be missing from MediaProfiles.h. As such, we can't rely upon
> +       having the CAMCORDER_QUALITY_* enums for those profiles. As such
> +       we need to map the profiles to a name by matching the width and
> +       height of the video resolution to our configured values. */
> +    for (int q = highestKnownQuality + 1; q <= CAMCORDER_QUALITY_LIST_END; ++q) {

Is 'q = highestKnownQuality + 1' a guarantee?

@@ +331,5 @@
> +      }
> +
> +      const ICameraControl::Size& s = profile->GetVideo().GetSize();
> +      size_t match = 0;
> +      for (; match < ProfileListDetectSize; ++match) {

for (size_t match = 0; ...) { // ??

@@ +347,5 @@
> +
> +      DOM_CAMERA_LOGI("Profile %d '%s' supported by platform\n",
> +                      q, ProfileListDetect[match].name);
> +      profile->mName.AssignASCII(ProfileListDetect[match].name);
> +      profiles->Put(profile->GetName(), profile);

Put this inside the for-loop, along with the break?

::: dom/camera/GonkRecorderProfiles.def
@@ +41,2 @@
>   *
>   * See bug 804359.

If this fixes the issue, you can remove this bug number reference.

Comment 16

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

Attachment: Detect profiles at runtime, v4

Fourth time is the charm...

Comment 17

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

(In reply to Mike Habicher [:mikeh] from comment #15)
> Comment on attachment 8529833 [details] [diff] [review]
> Detect profiles at runtime, v3
> 
> Review of attachment 8529833 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +323,5 @@
> > +       be missing from MediaProfiles.h. As such, we can't rely upon
> > +       having the CAMCORDER_QUALITY_* enums for those profiles. As such
> > +       we need to map the profiles to a name by matching the width and
> > +       height of the video resolution to our configured values. */
> > +    for (int q = highestKnownQuality + 1; q <= CAMCORDER_QUALITY_LIST_END; ++q) {
> 
> Is 'q = highestKnownQuality + 1' a guarantee?
> 

Yes. CAMCORDER_QUALITY_UNKNOWN is -1 and the floor of the value. +1 ensures it is at least 0, which is CAMCORDER_QUALITY_LIST_START. Even if by some mistake highestKnownQuality was >= CAMCORDER_QUALITY_LIST_END, that would just mean we skip this loop entirely.

I suppose somebody could reorder the enums everyone agrees on, or add new ones before say 1080p, but I think we should tell them that's wrong rather than support such bad behaviour :).

Comment 18

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

Attachment: Detect profiles at runtime, v4.1

Change _UNKNOWN to _LIST_START - 1.

Comment 19

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

Attachment: Detect profiles at runtime, v5

Get building on the emulator (doesn't like MOZ_ARRAY_LENGTH on anonymous structs).

Comment 20

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

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c926a6119d92

Comment 21

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

Comment on attachment 8529900 [details] [diff] [review]
Detect profiles at runtime, v5

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

r+ with the issues below addressed.

::: dom/camera/GonkRecorderProfiles.cpp
@@ +300,5 @@
>  GonkRecorderProfile::GetProfileHashtable(uint32_t aCameraId)
>  {
>    ProfileHashtable* profiles = sProfiles.Get(aCameraId);
>    if (!profiles) {
>      profiles = new ProfileHashtable;

At the risk of being pedantic, please add some parenthesis after 'new ProfileHashtable'.

http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-name-make-a-difference-with-new

@@ +338,5 @@
> +      const ICameraControl::Size& s = profile->GetVideo().GetSize();
> +      size_t match;
> +      for (match = 0; match < ProfileListDetectSize; ++match) {
> +        const ProfileConfigDetect& p = ProfileListDetect[match];
> +        if (s.width == p.width && s.height == p.height) {

I like your solution to this, but should we be worried about hitting a camera that supports, e.g., 800x480 *and* 768x480 recording?

::: dom/camera/GonkRecorderProfiles.def
@@ +31,5 @@
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_QCIF,    "qcif" )
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_CIF,     "cif"  )
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_480P,    "480p" )
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_720P,    "720p" )
> +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_1080P,   "1080p")

Please remove the extra spaces (but keep the strings and closing parentheses lined up).

Comment 22

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

Attachment: Detect profiles at runtime, v6 [carry r=mikeh]

(In reply to Mike Habicher [:mikeh] from comment #21)
> Comment on attachment 8529900 [details] [diff] [review]
> Detect profiles at runtime, v5
> 
> Review of attachment 8529900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the issues below addressed.
> 
> ::: dom/camera/GonkRecorderProfiles.cpp
> @@ +300,5 @@
> >  GonkRecorderProfile::GetProfileHashtable(uint32_t aCameraId)
> >  {
> >    ProfileHashtable* profiles = sProfiles.Get(aCameraId);
> >    if (!profiles) {
> >      profiles = new ProfileHashtable;
> 
> At the risk of being pedantic, please add some parenthesis after 'new
> ProfileHashtable'.
> 
> http://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-
> name-make-a-difference-with-new
> 

Done.

> @@ +338,5 @@
> > +      const ICameraControl::Size& s = profile->GetVideo().GetSize();
> > +      size_t match;
> > +      for (match = 0; match < ProfileListDetectSize; ++match) {
> > +        const ProfileConfigDetect& p = ProfileListDetect[match];
> > +        if (s.width == p.width && s.height == p.height) {
> 
> I like your solution to this, but should we be worried about hitting a
> camera that supports, e.g., 800x480 *and* 768x480 recording?
> 

Expanded comments to discuss this. I don't think this is a concern because there should only be one enum that maps to WVGA despite the fact that multiple resolutions in theory could be supported. Even if there are multiple profiles, we degrade gracefully to use the last profile discovered.

> ::: dom/camera/GonkRecorderProfiles.def
> @@ +31,5 @@
> > +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_QCIF,    "qcif" )
> > +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_CIF,     "cif"  )
> > +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_480P,    "480p" )
> > +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_720P,    "720p" )
> > +DEF_GONK_RECORDER_PROFILE(CAMCORDER_QUALITY_1080P,   "1080p")
> 
> Please remove the extra spaces (but keep the strings and closing parentheses
> lined up).

Done.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a58a47410707

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9ce5720d35f7

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ce5720d35f7