968644 - [Camera][Gecko] setParameter/getParameter API for general extensions

Status: RESOLVED WONTFIX

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

Description

[leo.bugzilla.gecko]

Expose set/get camera parameter to JS:
- setParameter(key, value)
- getParameter(key)

These APIs are needed to support the vendor-specific features.

e.g. vendor's own camera solution needs some setter/getter API for the solution.
But because it's not a general feature, it's not reasonable to make it a new API.
In this case, the vendor's camera app can use this API.

Comment 1

[leo.bugzilla.gecko]

I think it needs the below APIs.
 - setParameter(String key, int value)
 - setParameter(String key, String value)
 - getParameter(String key)    //returns String value
 - getIntParameter(String key) //returns int value

How about this?

Comment 2

[leo.bugzilla.gecko]

It's some ambiguous between int parameter case and string parameter case
I think it's better to make their name different with each other to
 - setParameter(String key, String value) and
 - setIntParameter(String key, int value)

Comment 3

[leo.bugzilla.gecko]

Mike, What do you think about this issue?
 - Do you agree with needs of this API?
 - If so, how is my proposal?

In case of MADAI, The APIs like this are needed.

Comment 4

[Youngwoo Jo]

Attachment: SetParameter-GetParameter-for-int-and-string.patch, v1

The Patch for the below APIs
 - void SetParameter(DOMString aKey, DOMString aValue)
 - void SetParameter(DOMString aKey, long aValue)
 - DOMString GetParameter(DOMString aKey)
 - long GetIntParameter(DOMString aKey)

Comment 5

[Youngwoo Jo]

I added the check if the owner window is a certified app.
So although it makes it exposed to a privileged app and a hosted app,
they can not use these APIs and can not abuse them.

Comment 6

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

Comment on attachment 8380581 [details] [diff] [review]
SetParameter-GetParameter-for-int-and-string.patch, v1

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

General feedback: when submitting a patch, please make sure you include at least 8 lines of context. You can do this by adding the following lines to your ~/.hgrc file:

[diff]
git = 1
showfunc = 1
unified = 8

Overall this looks good, but see my suggested changes to CameraControl.webidl.

::: dom/camera/DOMCameraControl.h
@@ +116,5 @@
>                         ErrorResult& aRv);
> +  void SetParameter(const nsAString& aKey, const nsAString& aValue, ErrorResult& aRv);
> +  void SetParameter(const nsAString& aKey, int32_t aValue, ErrorResult& aRv);
> +  void GetParameter(const nsAString& aKey, nsString& aValue, ErrorResult& aRv);
> +  int32_t GetIntParameter(const nsAString& aKey, ErrorResult& aRv);

See my suggested changes to CameraControl.webidl.

@@ +166,5 @@
>  
>    nsresult Set(JSContext* aCx, uint32_t aKey, const JS::Value& aValue, uint32_t aLimit);
>    nsresult Get(JSContext* aCx, uint32_t aKey, JS::Value* aValue);
> +
> +  bool CheckCertifiedAppPermission();

Please rename this to IsAppCertified(), but I think with my suggested change to CameraControl.webidl, it won't be needed anymore.

::: dom/camera/GonkCameraControl.cpp
@@ +478,5 @@
> +nsGonkCameraControl::Set(const nsAString& aKey, const nsAString& aValue)
> +{
> +  nsresult rv = mParams.Set(aKey, aValue);
> +  if (NS_FAILED(rv)) {
> +    DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv);

Change %s to '%s' to make any erronous whitespace in the key easy to see.

@@ +495,5 @@
> +nsGonkCameraControl::Set(const nsAString& aKey, int32_t aValue)
> +{
> +  nsresult rv = mParams.Set(aKey, aValue);
> +  if (NS_FAILED(rv)) {
> +    DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv);

Change %s to '%s' to make any erronous whitespace in the key easy to see.

@@ +499,5 @@
> +    DOM_CAMERA_LOGE("Camera parameter aKey=%s failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv);
> +    return rv;
> +  }
> +  return PushParameters();
> +}

Please create a new version of SetAndPush() that takes a const nsAString& aKey value, and call that from your two new setters.

::: dom/camera/GonkCameraParameters.cpp
@@ +687,5 @@
> +GonkCameraParameters::GetTranslated(const nsAString& aKey, nsAString& aValue)
> +{
> +  const char* val;
> +  nsresult rv = GetImpl(NS_ConvertUTF16toUTF8(aKey).get(), val);
> +  if (NS_FAILED(rv)) {

Change to:
  if (NS_WARN_IF(NS_FAILED(rv))) {

::: dom/webidl/CameraControl.webidl
@@ +330,5 @@
>                          optional CameraSetConfigurationCallback onSuccess,
>                          optional CameraErrorCallback onError);
> +
> +  /* sets a string parameter(aKey and aValue) to a camera device directly;
> +     aKey and aValue can be camera device specific.

Change this to: "sends an opaque, platform-specific parameter directly to the camera without interpretation or validation; using this method to change a setting that is already exposed by its own method (e.g. focusMode, flashMode, ISO) may cause problems, and is not recommended."

@@ +334,5 @@
> +     aKey and aValue can be camera device specific.
> +
> +     The following cases will throw an exception.
> +      - If setParameter with a string parameter failed
> +      - aKey and aValue are not available in a camera device

I don't think it is possible to tell when setting is a particular key is supported; the same with a particular value. At the camera library level, I think setParameters() ignores unrecognized keys and values.

@@ +335,5 @@
> +
> +     The following cases will throw an exception.
> +      - If setParameter with a string parameter failed
> +      - aKey and aValue are not available in a camera device
> +      - setParameter is not permitted in a caller app

What is a "caller app"?

@@ +338,5 @@
> +      - aKey and aValue are not available in a camera device
> +      - setParameter is not permitted in a caller app
> +  */
> +  [Throws]
> +  void setParameter(DOMString aKey, DOMString aValue);

Take a look at this: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#AvailableIn

I think tagging these methods as [AvailableIn=CertifiedApps] will make your implementation much simpler.

@@ +349,5 @@
> +      - aKey and aValue are not available in a camera device
> +      - setParameter is not permitted in a caller app
> +  */
> +  [Throws]
> +  void setParameter(DOMString aKey, long aValue);

This should be combined with the above as a single method:

  void setParameter(DOMString key, (DOMString or long)value);

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types

@@ +352,5 @@
> +  [Throws]
> +  void setParameter(DOMString aKey, long aValue);
> +
> +  /* returns the value of a string parameter from a camera device directly;
> +     aKey and the return value can be camera device specific.

Change this to: "returns the value of a platform-specific parameter directly from the camera without interpretation or validation; using this method to query a setting that is already exposed by its own method (e.g. focusMode, flashMode, ISO) may cause problems and is not recommended."

@@ +371,5 @@
> +      - aKey is not available in a camera device
> +      - getParameter is not permitted in a caller app
> +  */
> +  [Throws]
> +  long getIntParameter(DOMString aKey);

The above two should be combined as a single method:

  (DOMString or long) getParameter(DOMString key);

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Union_types

Comment 7

[Youngwoo Jo]

Attachment: SetParameter-GetParameter-for-int-and-String.patch, v2

I've modified according to your comments.
However, while combining getParameter(key) and getIntParameter(key) to (long or DOMString) getParameter(key) using an union type, I met a problem.
It's caused by the ambiguous signature between being called for long and for string. In Gonk level implementation, I was not able to decide if it should be mapped to camera HAL for long or something for string.
So, if I make your suggestion real, it should try both of calls to long type and string type, and then it should pick the succeed one. I think it's very poor.
What do you think about this case?

Comment 8

[Youngwoo Jo]

And now I have to check permission part more and add the test scripts.

Comment 9

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

(In reply to Youngwoo Jo from comment #7)
> 
> However, while combining getParameter(key) and getIntParameter(key) to (long
> or DOMString) getParameter(key) using an union type, I met a problem.
> It's caused by the ambiguous signature between being called for long and for
> string. In Gonk level implementation, I was not able to decide if it should
> be mapped to camera HAL for long or something for string.

In this case, I could call the ICameraControl string version and do strtol() on the result. If it's an integer, set the return value for that case, else set it as a string.

Comment 10

[Youngwoo Jo]

Attachment: SetParameter-GetParameter-API.patch, v3

I understood your opinion.
I became to have a different idea, which is that the interfaces of these APIs should be modified.
If Android::CameraParameters.set/get for all the types are implemented based on the string type, I think that only set/get API for string is enough to provide all the functionalities to the app-side.
So I think it's better to remove the APIs related to the integer type.
What do you think about this?
And I updated the new patch.

Comment 11

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

Comment on attachment 8381499 [details] [diff] [review]
SetParameter-GetParameter-for-int-and-String.patch, v2

When adding updated versions of patch, please tag their descriptions with a suitable version number; it makes it easier to sort out interdiffs.

Comment 12

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

Comment on attachment 8381499 [details] [diff] [review]
SetParameter-GetParameter-for-int-and-String.patch, v2

(I should get the version right, too.)

Comment 13

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

Comment on attachment 8381943 [details] [diff] [review]
SetParameter-GetParameter-API.patch, v3

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

This is getting close.

djf, what do you think? Is it worth having getParameter() return (long or DOMString), so that the '+'-operator in JS works as expected? (See my comments on dom/webidl/CameraControl.webidl.)

::: dom/camera/DOMCameraControl.h
@@ +164,5 @@
>  
>    nsresult Set(JSContext* aCx, uint32_t aKey, const JS::Value& aValue, uint32_t aLimit);
>    nsresult Get(JSContext* aCx, uint32_t aKey, JS::Value* aValue);
>  
> +  bool IsAppCertified();

With Get/SetParameter() tagged as [AvailableIn=CertifiedApps], this should no longer be required and can be removed.

::: dom/camera/GonkCameraControl.cpp
@@ +338,5 @@
> +nsGonkCameraControl::SetAndPush(const nsAString& aKey, const T& aValue)
> +{
> +  nsresult rv = mParams.Set(aKey, aValue);
> +  if (NS_FAILED(rv)) {
> +    DOM_CAMERA_LOGE("Camera parameter aKey=\'%s\' failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv);

The backslashes are not required, please remove them. Also, this line is very long; please break it before the NS_Convert call and indent the next line by 2 spaces.

e.g.
  if (NS_FAILED(rv)) {
    DOM_CAMERA_LOGE("Camera parameter aKey='%s' failed to set (0x%x)\n",
      NS_ConvertUTF16toUTF8(aKey).get(), rv);
    return rv;
  }

@@ +492,5 @@
> +  if (NS_FAILED(rv)) {
> +    DOM_CAMERA_LOGE("Camera parameter aKey=\'%s\' failed to set (0x%x)\n", NS_ConvertUTF16toUTF8(aKey).get(), rv);
> +    return rv;
> +  }
> +  return PushParameters();

This entire function can be reduced to a call to 'return SetAndPush(aKey, aValue);'

::: dom/webidl/CameraControl.webidl
@@ +343,5 @@
> +     a setting that is already exposed by its own method (e.g. focusMode,
> +     flashMode, ISO) may cause problems and is not recommended.
> +  */
> +  [AvailableIn=CertifiedApps, Throws]
> +  DOMString getParameter(DOMString key);

The reason why I want to support a union of (long or DOMString) as the return type is because of how Javascript's '+'-operator handles strings vs. numbers. If this method always returns strings, then:

  var x = CameraControl.getParameter("parameter-that-returns-a-numeric-value"); // e.g. 10
  var y = x + 1; // because x is a string, 1 --> "1"
  dump(y); // prints "101"

Comment 14

[David Flanagan [:djf]]

Mike,

Thanks for asking.  I can't think, offhand, of a DOM method that can return an integer or a string.  JS programmers are pretty used to calling parseInt() when we call getAttribute() on an HTML element, when we query a CSS style, or even when we use the new dataset parameter

  elt.dataset.x = 1;   // integer is converted to a string
  var x = parseInt(elt.dataset.x);

So it is fine with me if getParameter() always returns a string. In a way, it even makes the API simpler to understand.

This is probably a discussion for a followup bug, but has there been any thought about having a permission that controls use of specific vendor parameters? Like the permission "cameraparams:x,y,z" would allow use of 'x', 'y',and 'z' as keys for setParameter() and getParameter(). This would increase auditability, I suppose. If we did that could we open the API up to privileged apps?

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8381943 [details] [diff] [review]
SetParameter-GetParameter-API.patch, v3

Forgot to change the feedback flag.

Comment 16

[[:fabrice] Fabrice Desré]

Did we actually agree to land this kind of open api? I don't see any discussion on dev-webapi about that change.

Comment 17

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

(In reply to Fabrice Desré [:fabrice] from comment #16)
>
> Did we actually agree to land this kind of open api? I don't see any
> discussion on dev-webapi about that change.

Apologies, I didn't realize any more follow-up was needed. I'll drop this into dev-webapi in the morning.

Comment 18

[Youngwoo Jo]

Attachment: SetParameter-GetParameter-API.patch, v4

I removed IsAppCertified method and modified the patch by Mikeh's review.

Comment 19

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

We have decided not to follow this approach.