[Camera] Negative picture/video rotation values are incorrectly calculated

RESOLVED FIXED in Firefox 28

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 affected, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

Attachments

(2 attachments)

Copied from bug 947956 comment 56:

The code in Gecko to round the angle up to the nearest 90-degree increment doesn't correctly handle negative values. Here's the math:

  uint32_t r = static_cast<uint32_t>(aTakePicture->mRotation);
  r += mCameraHw->GetSensorOrientation(GonkCameraHardware::OFFSET_SENSOR_ORIENTATION);
  r %= 360;
  r += 45;
  r /= 90;
  r *= 90;
  SetParameter(CameraParameters::KEY_ROTATION, nsPrintfCString("%u", r).get());

So if aTakePicture->mRotation = -90 and GetSensorOrientation() returns 0, then:
  r %= 360 == -90
  r += 45 == -45 // this is the bug
  r /= 90 == 0
  r *= 90 == 0 // INCORRECT

The solution is to make sure the addition step reflects the sign of the argument, so:
  r %= 360 == -90
  r -= 45 == -135
  r /= 90 == -1
  r *= 90 == -90 // correct

Needs to be 1.3+ since it blocks a blocker.
1.3+ blocking a blocker
blocking-b2g: 1.3? → 1.3+
Dave, can you review this quickly? It's aimed at 1.3 and incorporates your feedback from reviewing bug 909542, with the addition of making sure the result is positive.
Attachment #8364443 - Flags: review?(dhylands)
Attachment #8364443 - Flags: feedback?(rexboy)
Comment on attachment 8364443 [details] [diff] [review]
Bug 963142 - fix negative-rotation math error in takePicture()

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

Wouldn't it be easier to get r positive and then not worry about + verus -45?

::: dom/camera/GonkCameraControl.cpp
@@ +1004,4 @@
>    r *= 90;
> +  if (r < 0) {
> +    r += 360;
> +  }

You could also do:
r %= 360; // range -359 to +359 (this step isn't needed if r never goes more negative than -359)
r += 360; // range 1 to 719
r %= 360; // range 0 to 359
r += 45;  // range 45 to 404
r /= 90;  // range 0 to 4
r %= 4;   // range 0 to 3
r *= 90;  // range 0 to 270

and get rid of all of the conditionals. Or for a slightly more optimal solution:

if (r < 0) {
  r %= 360; // range -359 to +359 (this step isn't needed if r never goes more negative than -359)
  r += 360; // range 1 to 719
}
r %= 360; // range 0 to 359
r += 45;  // range 45 to 404
r /= 90;  // range 0 to 4
r %= 4;   // range 0 to 3
r *= 90;  // range 0 to 270
Attachment #8364443 - Flags: review?(dhylands) → review+
For sanity, I wrote the following program:

#include <stdio.h>
int main(int argc, char **argv)
{
  int angle;
  int r1, r2, r;

  for (angle = -360; angle <= 360; angle++) {
    r = angle;
     if (r >= 0) {
       r += 45;
    } else {
      r -= 45;
    }
    r /= 90;
    r %= 4;
    r *= 90;
    if (r < 0) {
      r += 360;
    }
    r1 = r;

    r = angle;
    r %= 360; // range -359 to +359 (this step isn't needed if r never goes more negative than -359)
    r += 360; // range 1 to 719
    r %= 360; // range 0 to 359
    r += 45;  // range 45 to 404
    r /= 90;  // range 0 to 4
    r %= 4;   // range 0 to 3
    r *= 90;  // range 0 to 270
    r2 = r;

    if (r1 != r2) {
      printf("angle = %d r1 = %d r2 = %d\n", angle, r1, r2);
    }
  }
  return 0;
}

and it produced the following output:

angle = -315 r1 = 0 r2 = 90
angle = -225 r1 = 90 r2 = 180
angle = -135 r1 = 180 r2 = 270
angle = -45 r1 = 270 r2 = 0

so the only difference between the 2 algorithims is how the 45's get rounded (one rounds up one rounds down).

Since -315 is really the same as 45, I'd expect it produce the same answer which my proposed algorithim does. Yours doesn't.
Thanks for the review. To address your feedback, I think the API should reflect the sign of the rotation when rounding whenever possible. Theoretically, this really only affects 4/719 = 0.6% of all integer cases, but practically, an artificially-generated angle is much more likely to be a factor of 45 than an arbitrary value, and if the sign is there, we should probably respect it.
Status: NEW → ASSIGNED
Summary: [Camera] Negative picture rotation values are incorrectly calculated → [Camera] Negative picture/video rotation values are incorrectly calculated
I should have checked sooner, but the front-facing camera has the same problem when recording. Readying an additional patch.
And here's the second part. Should have caught this first time around.

With this patch, and the negative orientations from bug 947965, Helix takes pictures and records videos correctly in all orientations on both cameras. *phew*
Attachment #8364738 - Flags: review?(dhylands)
Comment on attachment 8364738 [details] [diff] [review]
Bug 963142 - fix negative-rotation math error in startRecording()

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

Looks good.
Attachment #8364738 - Flags: review?(dhylands) → review+
https://hg.mozilla.org/releases/mozilla-aurora/rev/524481b53bec
https://hg.mozilla.org/releases/mozilla-aurora/rev/376a27142809

b2g18 (v1.1) is only taking security fixes at this point, so marking that as wontfix. Not sure if this would be taken for a v1.2.x release or not, but you can request approval on the patches if you want.
Attachment #8364443 - Flags: feedback?(rexboy) → feedback+
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
You need to log in before you can comment on or make changes to this bug.