Bug 219732 - [Cocoa] Add Experimental VP8 support
Summary: [Cocoa] Add Experimental VP8 support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-10 00:36 PST by Jer Noble
Modified: 2020-12-11 18:37 PST (History)
10 users (show)

See Also:

Attachments
Patch (86.93 KB, patch)
2020-12-10 00:40 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (89.35 KB, patch)
2020-12-10 09:22 PST, Jer Noble 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (90.08 KB, patch)
2020-12-10 09:48 PST, Jer Noble 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (90.18 KB, patch)
2020-12-10 10:20 PST, Jer Noble 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (90.12 KB, patch)
2020-12-10 10:40 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (91.08 KB, patch)
2020-12-11 12:46 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (91.75 KB, patch)
2020-12-11 13:50 PST, Jer Noble 		eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (91.64 KB, patch)
2020-12-11 14:24 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Follow-up merge fix (1.66 KB, patch)
2020-12-11 18:33 PST, Jer Noble 		eric.carlson: review+
Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2020-12-10 00:36:26 PST 
[Cocoa] Add Experimental VP8 support
Comment 1 Radar WebKit Bug Importer 2020-12-10 00:37:07 PST 
<rdar://problem/72171055>
Comment 2 Jer Noble 2020-12-10 00:40:11 PST 
Created attachment 415843 [details]
Patch
Comment 3 Jer Noble 2020-12-10 09:22:22 PST 
Created attachment 415883 [details]
Patch
Comment 4 Jer Noble 2020-12-10 09:48:23 PST 
Created attachment 415889 [details]
Patch
Comment 5 Jer Noble 2020-12-10 10:20:45 PST 
Created attachment 415895 [details]
Patch
Comment 6 Jer Noble 2020-12-10 10:40:50 PST 
Created attachment 415900 [details]
Patch
Comment 7 Peng Liu 2020-12-10 12:59:52 PST 
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?
Comment 8 Jer Noble 2020-12-10 14:11:43 PST 
(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.
Comment 9 Sam Weinig 2020-12-11 11:37:02 PST 
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.
Comment 10 Jer Noble 2020-12-11 12:46:08 PST 
Created attachment 416036 [details]
Patch
Comment 11 Jer Noble 2020-12-11 13:50:43 PST 
Created attachment 416041 [details]
Patch
Comment 12 Eric Carlson 2020-12-11 14:06:45 PST 
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
Comment 13 Jer Noble 2020-12-11 14:21:36 PST 
(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.
Comment 14 Jer Noble 2020-12-11 14:24:28 PST 
Created attachment 416047 [details]
Patch for landing
Comment 15 EWS 2020-12-11 16:57:41 PST 
Committed r270720: <https://trac.webkit.org/changeset/270720>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416047 [details].
Comment 16 Jer Noble 2020-12-11 18:33:37 PST 
Reopening to attach new patch.
Comment 17 Jer Noble 2020-12-11 18:33:38 PST 
Created attachment 416083 [details]
Follow-up merge fix
Comment 18 Jer Noble 2020-12-11 18:37:56 PST 
Committed r270727: <https://trac.webkit.org/changeset/270727>