Camera - choose smallest thumbnail non-0x0 size

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

To save file space and conserve memory, we need to select the smallest non-0x0 thumbnail available on a platform that matches the aspect ratio of the desired photo.
Created attachment 677782 [details] [diff] [review]
Make camera default to smallest non-0x0 thumbnail size

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=e49f621070b8
Attachment #677782 - Flags: review?(kchen)
Comment on attachment 677782 [details] [diff] [review]
Make camera default to smallest non-0x0 thumbnail size

cjones, I've incorporated kanru's feedback.  Do you feel comfortable reviewing this in it's current form?  If so, we can probably get this landed before the weekend; if not, let me know and I'll r?kanru again.  Thanks.
Attachment #677782 - Flags: review?(kchen) → review?(jones.chris.g)
Comment on attachment 677782 [details] [diff] [review]
Make camera default to smallest non-0x0 thumbnail size

Trusting Kan-Ru's review on the camera parts here that I don't
know well.

>diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp

>+void
>+nsGonkCameraControl::GetParameter(uint32_t aKey, nsTArray<CameraSize>& aSizes)
>+{
>+  aSizes.Clear();
>+

I don't think we need this, do we?

>+  // The 'value' string is in the format "w1xh1,w2xh2,w3xh3,..."
>+  while (p) {
>+    s = aSizes.AppendElement();
>+    if (sscanf(p, "%dx%d", &s->width, &s->height) != 2) {
>+      DOM_CAMERA_LOGE("%s:%d : size tuple has bad format: '%s'\n", __func__, __LINE__, p);
>+      goto GetParameter_error;

I'm not sure about clearing the array on errors, but that doesn't
matter much.  However, |goto| is bad C++ style.  Let's just move the
Clear() up to here and return.  (The "right way" to do this is RAII,
but we don't need it here.)

>+    }
>+    // If we made it here, we read at least two numeric fields and an 'x', so
>+    //  skip ahead three characters to find the next separating commma.
>+    p = strchr(p + 3, ',');

I don't understand why we need the |p + 3|.  You already skip the
comma below.  Document why this is necessary or remove it.

>+    if (p) {
>+      // If not '\0', skip the comma

This should say "If nonnull", but I think that's clear from the |if
(p)| block we're in.  Let's just say "Skip the comma."

>+void
>+nsGonkCameraControl::SetupThumbnail(uint32_t aPictureWidth, uint32_t aPictureHeight, uint32_t aPercentQuality)
>+{

>+  nsTArray<CameraSize> thumbnailSizes;

nsAutoTArray

r=kanru,me with those addressed
Attachment #677782 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Comment on attachment 677782 [details] [diff] [review]
> Make camera default to smallest non-0x0 thumbnail size
> 
> Trusting Kan-Ru's review on the camera parts here that I don't
> know well.
> 
> >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
> 
> >+void
> >+nsGonkCameraControl::GetParameter(uint32_t aKey, nsTArray<CameraSize>& aSizes)
> >+{
> >+  aSizes.Clear();
> >+
> 
> I don't think we need this, do we?

In this instance we don't, but I want to start from a clean slate in the general case.

> >+  // The 'value' string is in the format "w1xh1,w2xh2,w3xh3,..."
> >+  while (p) {
> >+    s = aSizes.AppendElement();
> >+    if (sscanf(p, "%dx%d", &s->width, &s->height) != 2) {
> >+      DOM_CAMERA_LOGE("%s:%d : size tuple has bad format: '%s'\n", __func__, __LINE__, p);
> >+      goto GetParameter_error;
> 
> I'm not sure about clearing the array on errors, but that doesn't
> matter much.  However, |goto| is bad C++ style.  Let's just move the
> Clear() up to here and return.  (The "right way" to do this is RAII,
> but we don't need it here.)

Right.  This code was cut and pasted from another function (another variant of GetParameter()) which started out its life as something much more complex.  I'll clean this up in both places.  Thanks.

> >+    }
> >+    // If we made it here, we read at least two numeric fields and an 'x', so
> >+    //  skip ahead three characters to find the next separating commma.
> >+    p = strchr(p + 3, ',');
> 
> I don't understand why we need the |p + 3|.  You already skip the
> comma below.  Document why this is necessary or remove it.

It's not necessary, but we _know_ we can skip at least three characters here because sscanf(...) returned 2, which means the string pointed to by 'p' will at least be as long as (e.g.) "0x0".

> >+  nsTArray<CameraSize> thumbnailSizes;
> 
> nsAutoTArray

Done.

> r=kanru,me with those addressed
(In reply to Mike Habicher [:mikeh] from comment #4)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> > Comment on attachment 677782 [details] [diff] [review]
> > Make camera default to smallest non-0x0 thumbnail size
> > 
> > Trusting Kan-Ru's review on the camera parts here that I don't
> > know well.
> > 
> > >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
> > 
> > >+void
> > >+nsGonkCameraControl::GetParameter(uint32_t aKey, nsTArray<CameraSize>& aSizes)
> > >+{
> > >+  aSizes.Clear();
> > >+
> > 
> > I don't think we need this, do we?
> 
> In this instance we don't, but I want to start from a clean slate in the
> general case.
> 

Just change the API contract then.  But I certainly hope this code isn't used generally ...

> > >+    }
> > >+    // If we made it here, we read at least two numeric fields and an 'x', so
> > >+    //  skip ahead three characters to find the next separating commma.
> > >+    p = strchr(p + 3, ',');
> > 
> > I don't understand why we need the |p + 3|.  You already skip the
> > comma below.  Document why this is necessary or remove it.
> 
> It's not necessary, but we _know_ we can skip at least three characters here
> because sscanf(...) returned 2, which means the string pointed to by 'p'
> will at least be as long as (e.g.) "0x0".

This is an unnecessary optimization, and this kind of pointer math with not-entirely-obvious constants makes code harder to understand and maintain.  Just say no ;).
(In reply to Mike Habicher [:mikeh] from comment #4)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> >
> > >+  nsTArray<CameraSize> thumbnailSizes;
> > 
> > nsAutoTArray
> 
> Done.

Or not.  nsAutoTArray requires the number of elements to hold space for as a template parameter, something we don't know ahead of time.
Yes, that's what we want.  Pick a sane value that's a bit larger than the number of results you see from otoro/unagi drivers.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> (In reply to Mike Habicher [:mikeh] from comment #4)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> > > Comment on attachment 677782 [details] [diff] [review]
> > > Make camera default to smallest non-0x0 thumbnail size
> > > 
> > > Trusting Kan-Ru's review on the camera parts here that I don't
> > > know well.
> > > 
> > > >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
> > > 
> > > >+void
> > > >+nsGonkCameraControl::GetParameter(uint32_t aKey, nsTArray<CameraSize>& aSizes)
> > > >+{
> > > >+  aSizes.Clear();
> > > >+
> > > 
> > > I don't think we need this, do we?
> > 
> > In this instance we don't, but I want to start from a clean slate in the
> > general case.
> 
> Just change the API contract then.  But I certainly hope this code isn't
> used generally ...

Generally, as in if we ever decided to expose the available thumbnail sizes to JS, so that a camera application could choose a size.  Then this function would probably wind up getting called from other places.

Regardless, I'll remove the .Clear() call.

> > > >+    }
> > > >+    // If we made it here, we read at least two numeric fields and an 'x', so
> > > >+    //  skip ahead three characters to find the next separating commma.
> > > >+    p = strchr(p + 3, ',');
> > > 
> > > I don't understand why we need the |p + 3|.  You already skip the
> > > comma below.  Document why this is necessary or remove it.
> > 
> > It's not necessary, but we _know_ we can skip at least three characters here
> > because sscanf(...) returned 2, which means the string pointed to by 'p'
> > will at least be as long as (e.g.) "0x0".
> 
> This is an unnecessary optimization, and this kind of pointer math with
> not-entirely-obvious constants makes code harder to understand and maintain.
> Just say no ;).

I disagree, but you seem to feel more strongly about this than I do. :)  (Would sizeof("0x0") be more obvious?)
Saving 50 cycles or so in extremely cold code is just not the mental tax you'd put on people reading the code ;).
Created attachment 677853 [details] [diff] [review]
Make camera default to smallest non-0x0 thumbnail size, r=cjones,kanru

Incorporating feedback from reviews.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f9c3f3f6cbfc
Attachment #677782 - Attachment is obsolete: true
Comment on attachment 677853 [details] [diff] [review]
Make camera default to smallest non-0x0 thumbnail size, r=cjones,kanru

Adding r= from earlier reviews.
Attachment #677853 - Attachment description: Make camera default to smallest non-0x0 thumbnail size → Make camera default to smallest non-0x0 thumbnail size, r=cjones,kanru
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6134edeea902
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Part of the fix for bug 805114, which is bb+.  Blocks a blocker.
Blocks: 805114
blocking-basecamp: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce0906b1bc1d
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.