Attend XDA's Second Annual Developer Conference, XDA:DevCon 2014!
5,786,645 Members 38,801 Now Online
XDA Developers Android and Mobile Development Forum

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

Tip us?
 
mkasick
Old
(Last edited by mkasick; 27th February 2011 at 08:45 PM.) Reason: Attached stock EB13 kernel for flashing-back before upgrade.
#1  
Recognized Developer - OP
Thanks Meter 827
Posts: 470
Join Date: Aug 2009
Default [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:
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)
The Following 48 Users Say Thank You to mkasick For This Useful Post: [ Click to Expand ]
 
k0nane
Old
#2  
k0nane's Avatar
Recognized Developer
Thanks Meter 3,764
Posts: 3,981
Join Date: Feb 2008
Location: 127.0.0.1
Thank you! This will be in the ACS kernel team's kernel.





Have I helped? Enjoy my network's fast downloads? Hit the Thanks button!



Follow me on Twitter @k0nane / @publik0! Chat with the OUDHS: irc.freenode.net #oudhitsquad (webchat)
The Following User Says Thank You to k0nane For This Useful Post: [ Click to Expand ]
 
thomasskull666
Old
#3  
thomasskull666's Avatar
Senior Member
Thanks Meter 415
Posts: 1,552
Join Date: Sep 2010
Location: St. Louis

 
DONATE TO ME
Excellent, patching up as we speak.

Sent from my SPH-D700 using XDA App
*_Epic 4G_*
*_Sprint Galaxy Nexus_*
 
MysteryEmotionz
Old
#4  
MysteryEmotionz's Avatar
Senior Member
Thanks Meter 1,928
Posts: 5,545
Join Date: Sep 2010
Location: Massachussets

 
DONATE TO ME
Just wanted to be 3rd

Sent from my SPH-D700 using Tapatalk
If i helped you out in away please help me and BUY me a beer

Follow Me On Twitter @MysteryEmotionz

My Most Noticeable Work
Samsung Epic4g - Syndicate Frozen (Rom) 1.2 & EmotionlessBeast Resurrection (theme)
Samsung Admire - Admiral Best rom, Odin, CWM
LG E739 & E730 - MyEzOS 3.0 (rom & kernel )
LG E970 - B18c1 Kernel (Stock) - B20a1 (AOSP 4.2.2) - J32a2 (AOSP 4.3) EG8 ROM (LGUI)
 
thsisinsanity
Old
#5  
Member
Thanks Meter 2
Posts: 49
Join Date: Jun 2010
Cool, looks good.

Sent from my SPH-D700 using Tapatalk
 
Rodderik
Old
#6  
Rodderik's Avatar
Recognized Developer
Thanks Meter 1,312
Posts: 1,300
Join Date: Sep 2010

 
DONATE TO ME
patching myself too thx will be in Genocide Kernel 0.3a
Devices: EVO 4G LTE (pre-ordered), Epic 4g, Sprint 7" Galaxy Tab, HP TouchPad (CM9), Nook Color (CM7), Transform, Intercept

Epic 4G Kernel: Genocide EC05 Kernel v2.0|1.4GhzOC|RomManager|CustomUV|DUALBOOT
Galaxy Tab: [SPRINT][CDMA]Samsung Galaxy Tab (SPH-P100) Mega Development Starter Thread

http://devphone.org
The Following User Says Thank You to Rodderik For This Useful Post: [ Click to Expand ]
 
FoxRacR17
Old
#7  
Senior Member
Thanks Meter 2
Posts: 234
Join Date: Oct 2008
THANKS! leave it up to xda to do a better job then Samsung lol

Sent from my SPH-D700 using XDA App
 
tmspyder
Old
#8  
tmspyder's Avatar
Senior Member
Thanks Meter 13
Posts: 165
Join Date: Nov 2010
Sweet,
tested working perfect. the LSD trails in video are gone and no FC in sports mode. TY so much.
 
Rodderik
Old
#9  
Rodderik's Avatar
Recognized Developer
Thanks Meter 1,312
Posts: 1,300
Join Date: Sep 2010

 
DONATE TO ME
fix worked great! i just released genocide 0.3a with this patch included! thanks again!
Devices: EVO 4G LTE (pre-ordered), Epic 4g, Sprint 7" Galaxy Tab, HP TouchPad (CM9), Nook Color (CM7), Transform, Intercept

Epic 4G Kernel: Genocide EC05 Kernel v2.0|1.4GhzOC|RomManager|CustomUV|DUALBOOT
Galaxy Tab: [SPRINT][CDMA]Samsung Galaxy Tab (SPH-P100) Mega Development Starter Thread

http://devphone.org
The Following User Says Thank You to Rodderik For This Useful Post: [ Click to Expand ]
 
Tortel1210
Old
#10  
Retired Recognized Developer
Thanks Meter 295
Posts: 173
Join Date: Dec 2010
Gotta love 1 line fixes :)

Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes