Closed Bug 1029367 Opened 6 years ago Closed 6 years ago

[Camera][Gecko] Fix Map ISO functions

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:-, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S5 (4july)
feature-b2g -
tracking-b2g backlog

People

(Reporter: shinuk153, Assigned: mikeh)

References

Details

(Whiteboard: Map ISO functions)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

When I read ISO values from..

readonly attribute sequence<DOMString> isoModes



Actual results:

This attribute returns only 'auto' and 'hjr'.


Expected results:

This should return all ISO values including '100', '200', '400', etc.
Actually, I'm not sure why MapIsoFromGonk() and MapIsoToGonk() functions are needed.

I think mapping ISO values in GonkCameraParameters.cpp causes this issue.

All those ISO values from Camera HAL can be directly passed to Camera App.

i.e. isoModes = {auto, 100, 200, 400, 800} or {auto, iso100, iso200, iso400}

Please let me know your analysis or any proposal.
blocking-b2g: --- → 2.0?
feature-b2g: --- → -
Component: Gaia::Camera → General
Depends on: 965425
OS: All → Gonk (Firefox OS)
Priority: -- → P3
Hardware: All → ARM
Whiteboard: Map ISO functions
Flags: needinfo?(mhabicher)
Shinuk, the Map*() functions are intended to convert the values returned by the driver into something clean and consistent for JS. The current implementation is based on a library that returns values such as:

  {auto, ISO_HJR, ISO100, ISO200, ISO400, ...}

The value style is inconsistent, so we map them to {auto, hjr, 100, 200, 400, ...}.

Since ISO settings are not defined in the AOSP implementation[1] we based them on the libraries we had access to. If your library implements different values, please share them and we can adapt the function to handle them as well.

1. http://androidxref.com/4.4.3_r1.1/xref/frameworks/av/camera/CameraParameters.cpp
Flags: needinfo?(mhabicher)
General is not the correct component for this bug. Back to Camera. Please try another component if this is not a Gaia::Camera bug.
Component: General → Gaia::Camera
No known user impact, since this affects the underlying camera API, so not blocking.
blocking-b2g: 2.0? → backlog
Regarding my library implementation,

ISO values from Camera HAL {auto, ISO_HJR, 100, 200, 400, ...}

MapIsoFromGonk(const char* aIso, nsAString& aIsoOut) {

  if (strcmp(aIso, "ISO_HJR") == 0) {    ==> true
    aIsoOut.AssignASCII("hjr");
  } else if (strcmp(aIso, "auto") == 0) {  ==> true
    aIsoOut.AssignASCII("auto");
  } else {
    unsigned int iso;
    if (sscanf(aIso, "ISO%u", &iso) != 1) { ==> true(NS_ERROR_FAILURE, "ISO100" to unsigned int ?)
      return NS_ERROR_FAILURE;
    }
    aIsoOut.AppendInt(iso);
  }

thus, only JS gets isoModes = {auto, hjr};


I was wondering if your libraries had ISO values = {auto, ISO_HJR, ISO100, ISO200, ISO400, ...},

whether this code block can cause same issue or not.

    suppose aIso = "ISO100";

    sscanf(aIso, "ISO%u", &iso) == ?

Please let me know your analysis.

Thanks & Regards
Flags: needinfo?(mhabicher)
(In reply to shinuk from comment #5)
>
> I was wondering if your libraries had ISO values = {auto, ISO_HJR, ISO100,
> ISO200, ISO400, ...},

That's correct.

> whether this code block can cause same issue or not.
> 
>     suppose aIso = "ISO100";
> 
>     sscanf(aIso, "ISO%u", &iso) == ?
> 
> Please let me know your analysis.

It doesn't. See http://dxr.mozilla.org/comm-central/source/mozilla/dom/camera/test/test_camera_fake_parameters.html#117

It would be easier if your library were to implement the same format of return values as the others we've found, { "auto", "ISO_HJR", "ISO100", "ISO200", ... }, but if that is not an option, we can handle the different value types in Gecko.
Flags: needinfo?(mhabicher) → needinfo?(shinuk153)
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=aacc6cf6f33f

Though, as noted in comment 6, it would be nicer if the camera library returned formatted values we already handle.
Assignee: nobody → mhabicher
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to Mike Habicher [:mikeh] from comment #6)

> > whether this code block can cause same issue or not.
> > 
> >     suppose aIso = "ISO100";
> > 
> >     sscanf(aIso, "ISO%u", &iso) == ?
> > 
> > Please let me know your analysis.
> 
> It doesn't.

Correct.

Also, attached patch works for not 'ISO'-prefixed values.
Flags: needinfo?(shinuk153)
Comment on attachment 8446143 [details] [diff] [review]
WIP - handle different ISO mode format types, v1

If you would be so kind, sir.
Attachment #8446143 - Flags: review?(dhylands)
Comment on attachment 8446143 [details] [diff] [review]
WIP - handle different ISO mode format types, v1

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

::: dom/camera/GonkCameraParameters.cpp
@@ +195,1 @@
>      if (sscanf(aIso, "ISO%u", &iso) != 1) {

nit: Not sure if you care or not, but ISO31bc will pass this test. 

If do something like:

char term;
sscanf(aIso, "ISO%u%c", &iso, &term) != 1

then it will return 2 if there is extra input after the number.

@@ +258,5 @@
> +  uint32_t unrecognizedIsoModes = 0;
> +  for (uint32_t i = 0; i < isoModes.Length(); ++i) {
> +    if (!isoModes[i].EqualsASCII("ISO_HJR") &&
> +        !isoModes[i].EqualsASCII("auto")) {
> +      if (sscanf(isoModes[i].get(), "ISO%*u") == 1) {

%* doesn't contribute to the return value (so sscanf will return 0 always on the above)

See: http://linux.die.net/man/3/sscanf and search for suppression

@@ +260,5 @@
> +    if (!isoModes[i].EqualsASCII("ISO_HJR") &&
> +        !isoModes[i].EqualsASCII("auto")) {
> +      if (sscanf(isoModes[i].get(), "ISO%*u") == 1) {
> +        ++prefixedIsoModes;
> +      } else if (sscanf(isoModes[i].get(), "%*u") == 1) {

similarly, this will always return 0

@@ +265,5 @@
> +        ++bareIsoModes;
> +      } else {
> +        ++unrecognizedIsoModes;
> +      }
> +    }    

nit: trailing space

::: dom/camera/test/test_camera_fake_parameters.html
@@ +159,5 @@
> +      ok(cap.isoModes.indexOf("foo") == -1, "Unknown ISO mode 'foo' is ignored");
> +      ok(cap.isoModes.indexOf("ISObar") == -1, "Unknown ISO mode 'ISObar' is ignored");
> +      ok(cap.isoModes.indexOf("bar") == -1, "Unknown ISO mode 'bar' is ignored");
> +
> +      // test setters/getters for individual ISO modes

Presumably there is something wrong with the tests since it didn't detect the failure cases in the sscanf above.

I'd recommend that you fix the tests so that they fail, and then fix the sscanf.
Attachment #8446143 - Flags: review?(dhylands) → review-
Incorporate review feedback.
Attachment #8446143 - Attachment is obsolete: true
Attachment #8446734 - Flags: review?(dhylands)
Comment on attachment 8446734 [details] [diff] [review]
Handle different ISO mode format types, v2

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

Looks better.

I'd still like to see a test which passes in ISO100,100 and have it fail (I think that was what was missing before).

r=me with issues addressed.

::: dom/camera/GonkCameraParameters.cpp
@@ +261,5 @@
> +  uint32_t ignored;
> +  for (uint32_t i = 0; i < isoModes.Length(); ++i) {
> +    if (!isoModes[i].EqualsASCII("ISO_HJR") &&
> +        !isoModes[i].EqualsASCII("auto")) {
> +      if (sscanf(isoModes[i].get(), "ISO%u%c", &ignored, &ignored) == 1) {

%u wants an int ptr, and you're passing a uint32_t ptr. On a 32-bit architecure, these will be the same, but 64-bit they won't. So you should really just declare ignored as unsigned int.are ignored as unsigned int.

Also, if warning are turned on, you'll get:

> warning: format ‘%c’ expects argument of type ‘char *’, but argument 4 has type ‘unsigned int *’ [-Wformat]

so you really need another variable declared as a char and take the address of that.
Attachment #8446734 - Flags: review?(dhylands) → review+
This patch backs up a bit and instead of trying to guess which format the camera library is using, this just builds a hashtable of JS-facing values and their Gonk equivalents. Much simpler. It also incorporates the feedback from the earlier review.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=5dd66afec4d5
Attachment #8446734 - Attachment is obsolete: true
Attachment #8447303 - Flags: review?(dhylands)
Comment on attachment 8447303 [details] [diff] [review]
Handle different ISO mode format types, v3

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

r=me if you address the null dereference issue

::: dom/camera/GonkCameraParameters.cpp
@@ +160,5 @@
>  nsresult
>  GonkCameraParameters::MapIsoToGonk(const nsAString& aIso, nsACString& aIsoOut)
>  {
> +  nsCString* s;
> +  if (mIsoModeMap.Get(aIso, &s)) {

Boy - that sure is a weird API

@@ +254,2 @@
>      }
> +    *mIsoModes.AppendElement() = s;

It looks like AppendElement can return nullptr if the allocation fails:
http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1235

So you should probably check for non-null before dereferencing.

::: dom/camera/test/test_camera_fake_parameters.html
@@ +134,5 @@
> +      ok(cap.isoModes.indexOf("HJR") == -1, "ISO mode 'HJR' does not appear");
> +      ok(cap.isoModes.indexOf("ISO100") == -1, "ISO mode 'ISO100' does not appear");
> +      ok(cap.isoModes.indexOf("ISO200") == -1, "ISO mode 'ISO200' does not appear");
> +      ok(cap.isoModes.indexOf("ISO800") == -1, "ISO mode 'ISO800' does not appear");
> +      

nit: trailing space
Attachment #8447303 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #14)
> 
> It looks like AppendElement can return nullptr if the allocation fails:
> http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1235
> 
> So you should probably check for non-null before dereferencing.

AppendElement() can only fail for fallible arrays[1]. This array is not one of those.

1. http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#40
Fix nit, carry r+ forward.
Attachment #8447303 - Attachment is obsolete: true
Attachment #8447393 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5b9b93588f3e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.