Bug 219732 – [Cocoa] Add Experimental VP8 support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Cocoa] Add Experimental VP8 support

[Radar WebKit Bug Importer]

<rdar://problem/72171055>

[Jer Noble]

Created attachment 415843 [details]
Patch

[Jer Noble]

Created attachment 415883 [details]
Patch

[Jer Noble]

Created attachment 415889 [details]
Patch

[Jer Noble]

Created attachment 415895 [details]
Patch

[Jer Noble]

Created attachment 415900 [details]
Patch

[Peng Liu]

Comment on attachment 415900 [details]
Patch

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

> Source/ThirdParty/libwebrtc/ChangeLog:29
> +        * Source/webrtc/sdk/WebKit/WebKitVP8Decoder.h: Copied from Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.h.

Nit. I am afraid we need to update the change log generator. It often produces such information for newly added files.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:79
> +    } while (false);

Why do we need this "do ... while" loop trick?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:93
> +    auto createPixelFormatAttributes = [] (OSType pixelFormat, int32_t borderPixels) {

Nit. The space between [] and () can be removed.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:94
> +        auto createNumber = [] (int32_t format) -> CFNumberRef {

Ditto.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:1142
> +    uint16_t height;

No default value for height?

> Source/WebKit/GPUProcess/GPUProcess.cpp:304
> +        m_enableVP8Decoder = true;

Does this flag make sense for other ports?

[Jer Noble]

(In reply to Peng Liu from comment #7)
> Comment on attachment 415900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415900&action=review
> 
> > Source/ThirdParty/libwebrtc/ChangeLog:29
> > +        * Source/webrtc/sdk/WebKit/WebKitVP8Decoder.h: Copied from Source/WebCore/platform/graphics/cocoa/VP9UtilitiesCocoa.h.
> 
> Nit. I am afraid we need to update the change log generator. It often
> produces such information for newly added files.
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:79
> > +    } while (false);
> 
> Why do we need this "do ... while" loop trick?

It lets us do "early return"-style logic without having to create a self-running lambda.

> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:93
> > +    auto createPixelFormatAttributes = [] (OSType pixelFormat, int32_t borderPixels) {
> 
> Nit. The space between [] and () can be removed.
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:94
> > +        auto createNumber = [] (int32_t format) -> CFNumberRef {
> 
> Ditto.

This code is just being moved from one file to the other, so I wanted to avoid changing it at all.

> > Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:1142
> > +    uint16_t height;
> 
> No default value for height?

Will add.

> > Source/WebKit/GPUProcess/GPUProcess.cpp:304
> > +        m_enableVP8Decoder = true;
> 
> Does this flag make sense for other ports?

Probably not; but it won't affect them unless they set the ENABLE(VP9) flag.

[Sam Weinig]

Comment on attachment 415900 [details]
Patch

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

> Source/WebKit/ChangeLog:7
> +

Please fill in the ChangeLog.

[Jer Noble]

Created attachment 416036 [details]
Patch

[Jer Noble]

Created attachment 416041 [details]
Patch

[Eric Carlson]

Comment on attachment 416041 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:79
> +    } while (false);

You can use early returns instead of this loop.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:102
> +        auto borderPixelsValue = createNumber(32);

Why not use the `borderPixels` parameter?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:146
> +    OSStatus vtError = kVTVideoDecoderBadDataErr;

Nit: you could set this in a trailing `else` clause

[Jer Noble]

(In reply to Eric Carlson from comment #12)
> Comment on attachment 416041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416041&action=review
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:79
> > +    } while (false);
> 
> You can use early returns instead of this loop.

Ok.

> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:102
> > +        auto borderPixelsValue = createNumber(32);
> 
> Why not use the `borderPixels` parameter?

No idea why we have the borderPixels parameter in the first place, since this just used locally with a hard coded value.

> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitDecoderReceiver.cpp:146
> > +    OSStatus vtError = kVTVideoDecoderBadDataErr;
> 
> Nit: you could set this in a trailing `else` clause

Ok.

[Jer Noble]

Created attachment 416047 [details]
Patch for landing

[EWS]

Committed r270720: <https://trac.webkit.org/changeset/270720>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416047 [details].

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 416083 [details]
Follow-up merge fix

[Jer Noble]

Committed r270727: <https://trac.webkit.org/changeset/270727>