Post Reply

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

26th February 2011, 07:29 AM   |  #1  
OP Recognized Developer
Thanks Meter: 827
 
470 posts
Join Date:Joined: Aug 2009
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:
Code:
enum v4l2_iso_mode {
        ISO_AUTO = 0,
        ISO_50,
        ISO_100,
        ISO_200,
        ISO_400,
        ISO_800,
        ISO_1600,
        ISO_FIREWORKS, // Added since DI18.
        ISO_SPORTS,
        ISO_NIGHT,
        ISO_MOVIE,
        ISO_MAX,
};
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:
Code:
Before:          After:

ISO_AUTO:   0    ISO_AUTO:       0   
ISO_50:     1    ISO_50:         1   
ISO_100:    2    ISO_100:        2   
ISO_200:    3    ISO_200:        3   
ISO_400:    4    ISO_400:        4   
ISO_800:    5    ISO_800:        5   
ISO_1600:   6    ISO_1600:       6
ISO_SPORTS: 7    ISO_FIREWORKS:  7
ISO_NIGHT:  8    ISO_SPORTS:     8
ISO_MOVIE:  9    ISO_NIGHT:      9
                 ISO_MOVIE:     10
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)
Attached Files
File Type: txt epic_camera_fix-EB13.diff.txt - [Click for QR Code] (303 Bytes, 180 views)
File Type: tar epic_camera_fix-EB13.tar - [Click for QR Code] (5.49 MB, 195 views)
File Type: tar epic_stock-EB13.tar - [Click for QR Code] (5.50 MB, 33 views)
Last edited by mkasick; 27th February 2011 at 08:45 PM. Reason: Attached stock EB13 kernel for flashing-back before upgrade.
The Following 48 Users Say Thank You to mkasick For This Useful Post: [ View ]
26th February 2011, 07:48 AM   |  #2  
k0nane's Avatar
Recognized Developer
Flag 127.0.0.1
Thanks Meter: 3,767
 
3,981 posts
Join Date:Joined: Feb 2008
More
Thank you! This will be in the ACS kernel team's kernel.
The Following User Says Thank You to k0nane For This Useful Post: [ View ]
26th February 2011, 07:55 AM   |  #3  
thomasskull666's Avatar
Senior Member
Flag St. Louis
Thanks Meter: 415
 
1,552 posts
Join Date:Joined: Sep 2010
Donate to Me
More
Excellent, patching up as we speak.

Sent from my SPH-D700 using XDA App
26th February 2011, 08:11 AM   |  #4  
MysteryEmotionz's Avatar
Senior Member
Flag Massachussets
Thanks Meter: 1,931
 
5,547 posts
Join Date:Joined: Sep 2010
Donate to Me
More
Just wanted to be 3rd

Sent from my SPH-D700 using Tapatalk
26th February 2011, 08:18 AM   |  #5  
Member
Thanks Meter: 2
 
49 posts
Join Date:Joined: Jun 2010
Cool, looks good.

Sent from my SPH-D700 using Tapatalk
26th February 2011, 08:20 AM   |  #6  
Rodderik's Avatar
Recognized Developer
Thanks Meter: 1,312
 
1,300 posts
Join Date:Joined: Sep 2010
Donate to Me
More
patching myself too thx will be in Genocide Kernel 0.3a
The Following User Says Thank You to Rodderik For This Useful Post: [ View ]
26th February 2011, 09:14 AM   |  #7  
Senior Member
Thanks Meter: 2
 
234 posts
Join Date:Joined: Oct 2008
THANKS! leave it up to xda to do a better job then Samsung lol

Sent from my SPH-D700 using XDA App
26th February 2011, 09:58 AM   |  #8  
tmspyder's Avatar
Senior Member
Thanks Meter: 13
 
165 posts
Join Date:Joined: Nov 2010
More
Sweet,
tested working perfect. the LSD trails in video are gone and no FC in sports mode. TY so much.
26th February 2011, 10:13 AM   |  #9  
Rodderik's Avatar
Recognized Developer
Thanks Meter: 1,312
 
1,300 posts
Join Date:Joined: Sep 2010
Donate to Me
More
fix worked great! i just released genocide 0.3a with this patch included! thanks again!
The Following User Says Thank You to Rodderik For This Useful Post: [ View ]
26th February 2011, 11:48 AM   |  #10  
Retired Recognized Developer
Thanks Meter: 295
 
173 posts
Join Date:Joined: Dec 2010
More
Gotta love 1 line fixes :)

Post Reply Subscribe to Thread
Previous Thread Next Thread
Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes


Top Threads in Epic 4G Android Development by ThreadRank