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

Status: RESOLVED FIXED

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

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 1

[Preeti Raghunath(:Preeti)]

1.3+ blocking a blocker

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Bug 963142 - fix negative-rotation math error in takePicture()

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.

Comment 3

[Dave Hylands [:dhylands]]

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

Comment 4

[Dave Hylands [:dhylands]]

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.

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/integration/b2g-inbound/rev/f62ac0418ae6

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

I should have checked sooner, but the front-facing camera has the same problem when recording. Readying an additional patch.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Bug 963142 - fix negative-rotation math error in startRecording()

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*

Comment 9

[Dave Hylands [: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.

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

Thanks! Landed: https://hg.mozilla.org/integration/b2g-inbound/rev/15ec5afea92f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f62ac0418ae6
https://hg.mozilla.org/mozilla-central/rev/15ec5afea92f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.