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

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 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

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.

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*

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.

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

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

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.