Bug 193357 - Adopt new VCP SPI
Summary: Adopt new VCP SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 194039
Blocks:
  Show dependency tree / graph
 
Reported: 2019-01-11 11:49 PST by youenn fablet
Modified: 2019-04-03 10:38 PDT (History)
4 users (show)

See Also:

Attachments
Patch (9.16 KB, patch)
2019-01-11 11:57 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (12.81 KB, patch)
2019-01-28 21:33 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (15.33 KB, patch)
2019-01-29 09:39 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (10.41 KB, patch)
2019-04-01 14:36 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2019-04-02 08:45 PDT, youenn fablet 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) 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 youenn fablet 2019-01-11 11:49:11 PST 
Adopt new VCP SPI
Comment 1 youenn fablet 2019-01-11 11:51:44 PST 
<rdar://problem/43656651>
Comment 2 youenn fablet 2019-01-11 11:57:22 PST 
Created attachment 358924 [details]
Patch
Comment 3 youenn fablet 2019-01-28 21:33:55 PST 
Created attachment 360435 [details]
Patch
Comment 4 Eric Carlson 2019-01-29 05:56:35 PST 
Comment on attachment 360435 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:45
> +void testCompressionOutputCallback(void *, void *, OSStatus, VTEncodeInfoFlags, CMSampleBufferRef)

Nit: sanity checking the buffer and status parameters would make this even safer.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:73
> +        if (pixelFormat) {

Shouldn’t these tests be done before you use the values to create the dictionary?
Comment 5 youenn fablet 2019-01-29 09:03:12 PST 
(In reply to Eric Carlson from comment #4)
> Comment on attachment 360435 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360435&action=review
> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:45
> > +void testCompressionOutputCallback(void *, void *, OSStatus, VTEncodeInfoFlags, CMSampleBufferRef)
> 
> Nit: sanity checking the buffer and status parameters would make this even
> safer.

This cannot be called as we do not call encodeframe.
If we do not have the right encoder id, session creation will just fail.

> 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:73
> > +        if (pixelFormat) {
> 
> Shouldn’t these tests be done before you use the values to create the
> dictionary?

OK, I'll remove the if statements since they do not help much here.
Comment 6 youenn fablet 2019-01-29 09:39:40 PST 
Created attachment 360467 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-01-29 11:13:31 PST 
Comment on attachment 360467 [details]
Patch for landing

Clearing flags on attachment: 360467

Committed r240665: <https://trac.webkit.org/changeset/240665>
Comment 8 WebKit Commit Bot 2019-01-29 11:13:32 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2019-01-30 13:25:05 PST 
Re-opened since this is blocked by bug 194039
Comment 10 youenn fablet 2019-04-01 14:36:46 PDT 
Created attachment 366424 [details]
Patch
Comment 11 youenn fablet 2019-04-02 08:45:24 PDT 
Created attachment 366496 [details]
Patch
Comment 12 WebKit Commit Bot 2019-04-03 10:38:01 PDT 
Comment on attachment 366496 [details]
Patch

Clearing flags on attachment: 366496

Committed r243811: <https://trac.webkit.org/changeset/243811>
Comment 13 WebKit Commit Bot 2019-04-03 10:38:03 PDT 
All reviewed patches have been landed.  Closing bug.