[PATCH][KERNEL] EB13 camera video lag & sports mode FC fix

Attached is a kernel source patch that fixes the EB13 camera video lag and "Sports" scene mode force-close. It's actually the same kernel bug that's responsible for both.

Also attached is an Odin/redbend-flashable, otherwise stock EB13 kernel with this patch applied. It's intended for anyone who wishes to further test the fix and who is familiar with Odin and/or redbend_ua. For others, I'd recommend waiting until one of the custom kernels integrates the patch or until Sprint releases an official update.

I'll update this post in the morning with a workup of the bug (done, see below), since it's a bit interesting/illustrative. And yes, it's very simple and very silly. One of those things that really never should've happened. Thanks to everyone in the solutions thread for providing clues that were instrumental in locating it.

Bug details:

Shortly after the release of EB13 folks reported lag when recording videos, particularly in low light. Although not obviously related at the time, folks also reported that switching to "Sports" scene mode results in a force-closed (and in my experience, renders the screen inoperable as well, sigh). Folks in the solutions thread also reported that (i) these problems are new to EB13, they weren't present in DK28; and (ii) replacing the user-space camera components (app, libraries, etc.) with DK28 versions did not resolve the problems, implying they were likely due to one or more kernel bugs.

Unfortunately we do not have the DK28 source code to compare EB13 against, but at least knowing that these problems weren't present in DK28 helps narrow down the possible bug locations quite dramatically. Furthermore, it's quite likely that this bug was introduced rather recently, as I imagine it would've been caught by internal testing had it been present in say, December.

So with that in mind, I sorted the kernel source files by modification time and started looking for the most recently-changed files that might be relevant to the camera driver. "include/linux/videodev2_samsung.h" was the first hit with a modification date of 2/8, and indeed it's used by the camera driver ("drivers/media/video/victory/ce147.c", itself last modified on 12/1). Again, we don't have DK28 sources for comparison, but fortunately header files typically don't change too significantly and a comparison (diff) against the DI18 version was rather easy to follow.

And yes, a snippet of code stood out right away as rather strange, especially given the cirumstances of the problem:
For folks less-familiar with C, this code defines an enumerated type, basically a mapping of "descriptive labels" to numeric values; in this case: ISO_AUTO=0 ISO_50=1, ISO_100=2, etc. This enables kernel code to contain descriptive statements like "iso_mode = ISO_200;" instead of the more arbitrary "iso_mode = 3;".

Now, as the above comment (which I added, but the diff points it out) suggests, ISO_FIREWORKS is a new speed that was added to the middle of the enum since DI18. Seasoned C programmers will recognize that, this is often something that leads to trouble. To understand why, compare the "before" and "after" enum mappings:
The addition of ISO_FIREWORDS to the middle of the enum shifts the mapping of any labels below it, in this case ISO_SPORTS, ISO_NIGHT, and ISO_MOVIE. This isn't fatal, but it does mean that all code that uses the enum needs to be recompiled to reflect the new mapping. Often, kernel header files contain data types that are exclusive to the kernel, so any relevant code gets recompiled as part of the process of compiling a kernel.

But as it turns out, the entries in this enum are used in exactly one location (on the Epic anyways) in the kernel: as a case in a switch statement in the camera driver that sets the ISO mode in a camera hardware register. This means, assuming these values are used at all, they must be provided by a user-space library. In other words, the enum mapping is an integral part of the driver API, and not something that can be altered willy-nilly.

So basically, on EB13 when the Camera app goes to record a movie, it sets mode ISO_MOVIE (9), which the kernel interprets as ISO_NIGHT and sends to the camera hardware. Presumably ISO_NIGHT biases picture quality over shutter speed, hence the blurry laggy video when recording. Similarly, ISO_NIGHT => 8 => ISO_SPORTS (which no one noticed), and ISO_SPORTS => 7 => ISO_FIREWORKS. Except ISO_FIREWORKS isn't implemented, so the driver call fails which results in the force close. Oops!

The fix is fairly simple, just remove ISO_FIREWORKS from the enum. This allows the kernel and the user-space libraries to agree on the mapping. And since ISO_FIREWORKS isn't even implemented in the kernel, no harm can possibly come from it.

Finally, as I stated earlier, this is a bug that never should've happened, for two reasons. First, it was introduced into the Epic kernel source tree after DK28. Now, keep in mind that DK28 was effectively a Froyo "release candidate", especially given that it was packaged up as an OTA update and accidentally pushed to some handsets. Any changes made to the kernel post-DK28 should be limited to strict bug fixes only. The addition of ISO_FIREWORKS to the enum is not part of any bug fix (indeed it introduces one), rather one would consider it a "new feature". But "new features" shouldn't creep into stable code trees. This suggests poor code management.

Second, this alteration would've been a non-issue had ISO_FIREWORKS been appended to the end of the enum, just before ISO_MAX (which, presumably just reports the numer of entries in the enum as opposed to describing a particular mode). This would've assiged ISO_FIREWORKS to an unused value, instead of remapping existing values. Adding ISO_FIREWORKS to the middle of the enum is a particularly short-sighted choice, as it immediately renders any code that uses it unnecessarilly incompatible with new kernels. Adding ISO_FIREWORKS to the end preserves backwards compatibility, for free, with absolutely no downsides--why not do that instead?

So, in short, the failure to properly consider both of these issues, as well as the neglect to notice the change, bespeaks of an incompetent moment on the part of Samsung. If the change really was made on 2/8, one can't really blame Sprint for failing to pick it up during last-minute testing. I'm not sure how much this particular bug was a factor in Sprint pulling the EB13 update, but it's pretty embarassing that it made it out there in the first place.

Mirror links (does not require forum login):
epic_camera_fix-EB13.diff
epic_camera_fix-EB13.tar
epic_stock-EB13.tar (for flashing back to the stock EB13 kernel)