Closed
Bug 1029367
Opened 10 years ago
Closed 10 years ago
[Camera][Gecko] Fix Map ISO functions
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect, P3)
Tracking
(feature-b2g:-, tracking-b2g:backlog)
People
(Reporter: shinuk153, Assigned: mikeh)
References
Details
(Whiteboard: Map ISO functions)
Attachments
(1 file, 3 obsolete files)
8.79 KB,
patch
|
mikeh
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
General is not the correct component for this bug. Back to Camera. Please try another component if this is not a Gaia::Camera bug.
Updated•10 years ago
|
Component: General → Gaia::Camera
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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-
Assignee | ||
Comment 11•10 years ago
|
||
Incorporate review feedback.
Attachment #8446143 -
Attachment is obsolete: true
Attachment #8446734 -
Flags: review?(dhylands)
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
(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
Assignee | ||
Comment 16•10 years ago
|
||
Fix nit, carry r+ forward.
Attachment #8447303 -
Attachment is obsolete: true
Attachment #8447393 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5b9b93588f3e
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b9b93588f3e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•