Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the color attachment comparison bug #1449

Merged
merged 1 commit into from
Mar 18, 2014

Conversation

stelabouras
Copy link
Contributor

Fixes the bug of using the wrong YUV to RGB conversion table while displaying the video from the camera (GPUImageVideoCamera) or playing back a video using the GPUImageMovie class.

In shorts the comparison between two CFStringRef variables was the cause of the problem.

Instead of using CFStringCompare, there was a == comparison which didn't return the correct result.

What is troubling me the most though is that the exact same bug exists in Apple's sample code of 'Real-time Video Processing Using AVPlayerItemVideoOutput' as well, so be advised :)

The bug caused the red color to be displayed as orange (due to the fact that it was using the 709 conversion table instead of the 601 a.k.a. wrong YUV to RGB conversion), as seen from the screenshot below:

comparison

This bug also affected any rendered movie file from GPUImage that used GPUImageVideoCamera as source.

@tspecht
Copy link

tspecht commented Mar 16, 2014

+1!

BradLarson added a commit that referenced this pull request Mar 18, 2014
Fixed the color attachment comparison bug
@BradLarson BradLarson merged commit 63ac8e9 into BradLarson:master Mar 18, 2014
@BradLarson
Copy link
Owner

Thanks, appreciate it. Sorry I took so long to work on this, but as you can see I had a few other things going on.

BradLarson added a commit that referenced this pull request Mar 18, 2014
@BradLarson
Copy link
Owner

This actually introduced a crash on iOS 7, because when taking a photo, apparently the colorAttachments variable here is sometimes NULL. This causes the string comparison to fail. I default to the second colorspace, but you might need to check that I've selected the right one in the latest code in the repository.

@stelabouras
Copy link
Contributor Author

Oh, snap. I will look into it and I will let you know :) Good catch 👍

@stelabouras
Copy link
Contributor Author

After some extensive tests, it seems that the proper solution for this is to handle differently the luminance levels in the shader for photo and video.

I will make some fixes and issue a new pull request when it's ready 😄

@stelabouras
Copy link
Contributor Author

In the still picture mode, the YCbCr data is in different ranges than in video mode (Y in video is [16...235] while in picture it's [0...255]), so a different conversion matrix is required (reference here).

After comparing GPUImage photo output with the one of the default Camera app, we concluded that we will need the full range RGB to YCbCr (ITU-R BT.601) conversion matrix.

// BT.601 Full Range
const GLfloat kColorConversion601FullRange[] = {
    1.0,    1.0,    1.0,
    0.0,    -0.343, 1.765,
    1.4,    -0.711, 0.0,
};

I will issue a pull request about that as soon as possible.


We also noticed that there is a problem with the conversion matrices in video (BT.601 and BT.709).

As seen in the sample Apple app and in the reference article, we need to subtract 16 from the luminance component (Y). But applying the subtraction, still does not match the default Camera app's video output. This one needs some more thorough investigation, though 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants