Bug 122486 – Video -> pixel buffer output should not manage color spaces

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dean Jackson]

Pixel output from video (drawn into a canvas or uploaded to WebGL) should not attempt to manage the color space.

[Radar WebKit Bug Importer]

<rdar://problem/15172914>

[Dean Jackson]

Created attachment 213653 [details]
Patch

[Tim Horton]

Comment on attachment 213653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213653&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:866
> +    RetainPtr<CGImageRef> image = adoptCF(CGImageCreateCopyWithColorSpace(rawImage, CGColorSpaceCreateDeviceRGB()));

You're leaking the colorspace I think?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1284
> +        static CGColorSpaceRef deviceRGB = CGColorSpaceCreateDeviceRGB();

Wat. Maybe use deviceColorSpaceRef() in both of these places instead and avoid the leak/unnecessary static :D (you may have to include GraphicsContextCG.h, but that's definitely OK in this file).

[Tim Horton]

As a more general comment... what if the video is legitimately tagged with an unusual color space? Can you only do this retagging if the video's current colorspace == the "default" (GenericRGB I think you said today?). Similar to what imageWithColorSpace does.

[Dean Jackson]

Created attachment 213703 [details]
Patch

[Dean Jackson]

(In reply to comment #4)
> As a more general comment... what if the video is legitimately tagged with an unusual color space? Can you only do this retagging if the video's current colorspace == the "default" (GenericRGB I think you said today?). Similar to what imageWithColorSpace does.

I think we'll produce incorrect results in that case, but that's better than now, where we are producing incorrect results in every case.

The good news is that video elements should still display correctly. This is only affecting pixel reads from video.

My possibly incorrect hope is that if we move the engine to sRGB, then this code could probably be removed. The problem at the moment is that the pixels are being interpreted as sRGB but then lose that information (either when drawn into a canvas or uploaded to the GPU directly).

[Dean Jackson]

Created attachment 213704 [details]
Patch

[Tim Horton]

Comment on attachment 213704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213704&action=review

Indeed, hopefully this will all get cleaned up in the near future. r+ if one of the media folks agree for now.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:866
> +    CGImageRef rawImage = [m_imageGenerator.get() copyCGImageAtTime:CMTimeMakeWithSeconds(time, 600) actualTime:nil error:nil];

Vaguely intrigued why this isn't a retainptr too but there's really no reason for it to be!

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 213704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213704&action=review
> 
> Indeed, hopefully this will all get cleaned up in the near future. r+ if one of the media folks agree for now.

I'm fine with it, though I'd prefer to wrap the bare pointers with RetainPtrs.

[Dean Jackson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 213704 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213704&action=review
> > 
> > Indeed, hopefully this will all get cleaned up in the near future. r+ if one of the media folks agree for now.
> 
> I'm fine with it, though I'd prefer to wrap the bare pointers with RetainPtrs.

I'll do that, but it seems like extra work in this case, or at least extra code.

- add RetainPtr<CGImageRef> over just CGImageRef
- add adoptCF
- add .get()
- remove CGImageRelease

[Dean Jackson]

Committed r157133: <http://trac.webkit.org/changeset/157133>