Closed Bug 965425 Opened 6 years ago Closed 6 years ago

[Madai][Camera][Gecko] Expose ISO settings

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: dev-doc-needed, Whiteboard: ETA 3/7)

Attachments

(1 file, 4 obsolete files)

Expose ISO sensitivity settings to JS:
- capability-supported attribute
- current setting attribute
I want to implement this feature like Android.
ISO feature can be implemented using Parameters.set(String key, String value) API.
For example, in Android
  To set ISO value
    - Parameters.set("iso", "auto");
    - Parameters.set("iso", "ISO200");
    - Parameters.set("iso", "ISO800");
  To get ISO value
    - Parameters.get("iso") : returns the current ISO value, such as "auto", "ISO200"
  To get Capability
    - Parameters.get(/*key-for-iso-capability*/)

So we want to add set(key/value) API to support this feature.
However the pair of key and value should be a representative value.
So now I have to check the pair of key and value.
I have started implementing this feature for Gecko. The valid parameter values will be whatever is returned by the camera library, without the redundant "ISO" prefix. For example:

  Parameters.get(/*key-for-iso-capability*/) --> { "auto", "ISO100", "ISO200", "ISO400" }
  capabilities.getIsoModes() --> { "auto", "100", "200", "400" }

I have noticed that some implementations return an ISO mode of "HJR"--do you know what this value means?
Status: NEW → ASSIGNED
Are you implementing this API like setISO('auto') or set('iso', 'auto')
The first one is only for ISO but the second one is very extensible.
Because ISO is a very general feature, I think it's not a problem to be implemented as a new API like setISO('auto').

However, can you consider the the below APIs?
 - set(String key, int value)
 - set(String key, String value)
 - set(String key, boolean value) //optional
 - get(String key)    //returns String value
 - getInt(String key) //returns int value

These APIs can provide the various vendors with the method for extension.
I expect that almost vendors want to extend the APIs to set a parameter depending on the features supported by their own camera solution or h/w.
However, we can not add a new API whenever it's needed to support a vendor's camera driver specific features.
So we need to make the API general.

If you agree with this idea, do you think we should make the issue separated?
I found a link about HJR : http://www.dpreview.com/forums/post/50107202
This is an auto-iso mode supported by Google Android.
Assignee: mhabicher → hiro7998
I think it's better to make an ISO specific API like setISO('100')
So I made a new issue for general set/get API. (bug 968644)
Youngwoo,

Based on meeting held on 2/6 since Mike already has started and has a partial patch, he can take this up and complete it (gecko only)

Thanks
Hema
Assignee: hiro7998 → mhabicher
Sure, I had a misunderstanding about this.
Target Milestone: --- → 1.4 S1 (14feb)
blocking-b2g: 1.4? → ---
Summary: [Camera][Gecko] Expose ISO settings → [Madai][Camera][Gecko] Expose ISO settings
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
For my understanding, ISO is not in the list of Camera.Parameters - the API for Android developer[1][2].
So the key and the values for ISO setting is based on Qualcomm's implementation[3].
Because the key and the values are not in the list of Camera.Parameters, it means those values are vendor-dependence. Do we have any specifications or place to list the definitions of camera parameter? For example, if there is another chip-set vendor want to provide ISO setting capability. Is there any way they know the key-for-iso-capability definition? Also should we remove the redundant "ISO" prefix? What if the other chip-set vendor provide the value of ISO as "ISO-100".... and we just deal with "ISO100"....

In summary:
1. ISO is not in Android HAL/developer API definition. We might need to consider other chip-set vendor.
2. If we want to support this feature, is there any reference key and values for chip-set vendor? Or we just hidden and deal with everything inside gecko?


[1]: http://developer.android.com/reference/android/hardware/Camera.Parameters.html
[2]: http://androidxref.com/4.4.2_r1/xref/frameworks/av/camera/CameraParameters.cpp
[3]: http://androidxref.com/4.4.2_r1/xref/hardware/qcom/camera/QCamera2/HAL/QCameraParameters.cpp#199

(In reply to Mike Habicher [:mikeh] from comment #2)
> I have started implementing this feature for Gecko. The valid parameter
> values will be whatever is returned by the camera library, without the
> redundant "ISO" prefix. For example:
> 
>   Parameters.get(/*key-for-iso-capability*/) --> { "auto", "ISO100",
> "ISO200", "ISO400" }
>   capabilities.getIsoModes() --> { "auto", "100", "200", "400" }
> 
> I have noticed that some implementations return an ISO mode of "HJR"--do you
> know what this value means?
By the way, the file frameworks/base/libs/camera/CameraParameters.cpp in B2G is different from the file in original Android version. Code aurora adds their own customize codes. The file CameraParameters.cpp in the devices FlatFish has no ISO related definitions.
Attached patch WIP - expose ISO modes, v1 (obsolete) — Splinter Review
Attachment #8381449 - Flags: feedback?(dmarcos)
Attachment #8381449 - Flags: feedback?(dmarcos) → feedback?(dflanagan)
Comment on attachment 8381449 [details] [diff] [review]
WIP - expose ISO modes, v1

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

Mike,

This looks fine to me.  I'm surprised that there is only a webidl change for the capabilities object.  Is the property for setting and querying iso mode already defined in some other webidl file, or did you forget to add that change to the patch?

I like your approach of stripping "ISO" off of the identifiers.  Any thoughts on ctai's concerns about other chipsets?  I'd say that for hardware that doesn't support the QC standard, Gaia can use the generic setParameter method that is pending, and we can put device specific code in Gaia.

::: dom/camera/GonkCameraParameters.cpp
@@ +79,5 @@
>        return KEY_VIDEO_SIZE;
> +    case CAMERA_PARAM_ISOMODE:
> +      // Not every platform defines KEY_ISO_MODE;
> +      // for those that don't, we use the raw string key.
> +      return "iso";

From the comment, I would have expected an #ifdef KEY_ISO_MODE in the code.

@@ +159,5 @@
> +    unsigned int iso;
> +    if (sscanf(v.get(), "%u", &iso) != 1) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    aIsoOut = nsPrintfCString("ISO%u", iso);

Is any validation against the list of supported values required?  Or is it safe (and secure) to ISO followed by any arbitrary unsigned integer here?
Attachment #8381449 - Flags: feedback?(dflanagan) → feedback+
Attached patch WIP - expose ISO modes, v2 (obsolete) — Splinter Review
Latest version of the patch; this one actually exposes the .isoMode attribute so you can set it to something. :I

This patch applies on top of the patch in bug 977756.

TODO: mochitests using the fake parameters framework of bug 976802.
Attachment #8381449 - Attachment is obsolete: true
Attached patch Expose ISO modes, v3 (obsolete) — Splinter Review
This applies on top of the patch in bug 977756.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=c81fd9cf5f94
Attachment #8383995 - Attachment is obsolete: true
Attachment #8384989 - Flags: review?(dhylands)
Attachment #8384989 - Flags: feedback?(jdarcangelo)
Comment on attachment 8384989 [details] [diff] [review]
Expose ISO modes, v3

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

Just a couple of minor things...

::: dom/camera/DOMCameraCapabilities.cpp
@@ +132,5 @@
>    LOG_IF_ERROR(rv, CAMERA_PARAM_SUPPORTED_FLASHMODES);
>  
>    rv = aCameraControl->Get(CAMERA_PARAM_SUPPORTED_FOCUSMODES, mFocusModes);
>    LOG_IF_ERROR(rv, CAMERA_PARAM_SUPPORTED_FOCUSMODES);
> +  

nit: trailing space

::: dom/camera/GonkCameraParameters.cpp
@@ +155,5 @@
> +    aIsoOut = "ISO_HJR";
> +  } else if (aIso.EqualsASCII("auto")) {
> +    aIsoOut = "auto";
> +  } else {
> +    nsAutoCString v = NS_LossyConvertUTF16toASCII(aIso);

Do you mean NS_ConvertUTF16toUTF8 ? Or do you really want ASCII?

@@ +233,5 @@
> +    if (NS_SUCCEEDED(rv)) {
> +      v = mIsoModes.AppendElement();
> +    }
> +    rv = MapIsoFromGonk(isoModes[i].get(), *v);
> +  }

If I'm reading this right, you'll append an empty element if the last MapIsoFromGonk fails?

@@ +473,4 @@
>  
> +        return SetImpl(CAMERA_PARAM_PICTURE_DATETIME, dateTime);
> +      }
> +      break;

nit: break unnecessary after return

@@ +485,5 @@
> +        nsString s;
> +        s.AppendInt(aValue);
> +        return SetTranslated(CAMERA_PARAM_ISOMODE, s);
> +      }
> +      break;

nit: break unnecessary after return
Attachment #8384989 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #14)
> 
> ::: dom/camera/GonkCameraParameters.cpp
> @@ +155,5 @@
> > +    aIsoOut = "ISO_HJR";
> > +  } else if (aIso.EqualsASCII("auto")) {
> > +    aIsoOut = "auto";
> > +  } else {
> > +    nsAutoCString v = NS_LossyConvertUTF16toASCII(aIso);
> 
> Do you mean NS_ConvertUTF16toUTF8 ? Or do you really want ASCII?

Gonk doesn't seem to use non-ASCII characters, so it doesn't make a difference either way. I think the lossy converter makes it more clear what can get stuffed in there.

> @@ +233,5 @@
> > +    if (NS_SUCCEEDED(rv)) {
> > +      v = mIsoModes.AppendElement();
> > +    }
> > +    rv = MapIsoFromGonk(isoModes[i].get(), *v);
> > +  }
> 
> If I'm reading this right, you'll append an empty element if the last
> MapIsoFromGonk fails?

Yeah. Fixed.
Attached patch Expose ISO modes, v4 r=dhylands (obsolete) — Splinter Review
Address feedback from review, fix JB build breakage; carrying r+ forward.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=bb2129b7d477&showall=1
Attachment #8384989 - Attachment is obsolete: true
Attachment #8384989 - Flags: feedback?(jdarcangelo)
Attachment #8385363 - Flags: review+
Comment on attachment 8385363 [details] [diff] [review]
Expose ISO modes, v4 r=dhylands

jst, would you mind reviewing the DOM-facing parts of this patch? It exposes the ISO sensitivity attribute on CameraControl. DOM-facing changes are in:
- DOMCameraCapabilities.cpp/.h
- DOMCameraControl.cpp/.h
- CameraCapabilities.webidl
- CameraControl.webidl
Attachment #8385363 - Flags: review+ → review?(jst)
Rebased to no longer rely on the patch for bug 977756, since that change isn't working as expected.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=60f4a86a18b0&showall=1
Attachment #8385363 - Attachment is obsolete: true
Attachment #8385363 - Flags: review?(jst)
Attachment #8386899 - Flags: review?(jst)
Comment on attachment 8386899 [details] [diff] [review]
Expose ISO modes, v4.1 r=dhylands

r=jst
Attachment #8386899 - Flags: review?(jst) → review+
Whiteboard: ETA 3/7
https://hg.mozilla.org/mozilla-central/rev/26779ad44626
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1029367
You need to log in before you can comment on or make changes to this bug.