Bug 183912

Summary: Use libwebrtc ObjectiveC H264 encoder and decoder
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, ews-watchlist, jer.noble, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 183929    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Patch for landing
none
Patch
none
Patch none

Description youenn fablet 2018-03-22 12:52:36 PDT
This allows sticking closer to libwebrtc
Comment 1 youenn fablet 2018-03-22 13:02:13 PDT
Created attachment 336297 [details]
Patch
Comment 2 youenn fablet 2018-03-22 13:40:01 PDT
Created attachment 336306 [details]
Patch
Comment 3 youenn fablet 2018-03-22 14:34:10 PDT
Created attachment 336314 [details]
Patch
Comment 4 youenn fablet 2018-03-22 15:30:34 PDT
Created attachment 336318 [details]
Patch
Comment 5 Eric Carlson 2018-03-22 16:11:34 PDT
Comment on attachment 336318 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/WebKitUtilities.mm:35
> +#if! defined(WEBRTC_IOS)

Nit: the space is on the wrong side of the "!"

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoDecoderH264.mm:194
> +  Block_release(_callback);

Can _callback ever be NULL?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/RTCVideoEncoderH264.mm:485
> +  Block_release(_callback);

Ditto.

> Source/ThirdParty/libwebrtc/WebKit/0001-Adapting-libwebrtc-H264-codec.patch:6
> +From 0130a91f901af2c60c81c32565c531141bbd2b84 Mon Sep 17 00:00:00 2001
> +From: Youenn Fablet <youenn@apple.com>
> +Date: Thu, 22 Mar 2018 12:51:04 -0700
> +Subject: [PATCH] Adapting libwebrtc H264 codec
> +
> +---

Oops :-)

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:83
> +        if (!m_blackFrame || m_blackFrameWidth != frame.width() || m_blackFrameHeight != frame.height()) {
> +            CVPixelBufferRef pixelBuffer = nullptr;
> +            auto status = CVPixelBufferCreate(kCFAllocatorDefault, frame.width(), frame.height(), kCVPixelFormatType_420YpCbCr8Planar, nullptr, &pixelBuffer);
> +            ASSERT_UNUSED(status, status == noErr);
> +
> +            m_blackFrame = adoptCF(pixelBuffer);
> +            m_blackFrameWidth = frame.width();
> +            m_blackFrameHeight = frame.height();
> +
> +            status = CVPixelBufferLockBaseAddress(pixelBuffer, 0);
> +            ASSERT(status == noErr);
> +            void* data = CVPixelBufferGetBaseAddress(pixelBuffer);
> +            size_t yLength = frame.width() * frame.height();
> +            memset(data, 0, yLength);
> +            memset(static_cast<uint8_t*>(data) + yLength, 128, yLength / 2);
> +
> +            status = CVPixelBufferUnlockBaseAddress(pixelBuffer, 0);
> +            ASSERT(!status);
> +        }
> +        return m_blackFrame.get();

Nit: I know this isn't new, but it would be cleaner if it was in a blackFrame method, e.g. "if (muted())  return blackFrame();"
Comment 6 EWS Watchlist 2018-03-22 16:32:19 PDT
Comment on attachment 336318 [details]
Patch

Attachment 336318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7068032

New failing tests:
media/modern-media-controls/tracks-panel/tracks-panel-position-and-size.html
Comment 7 EWS Watchlist 2018-03-22 16:32:20 PDT
Created attachment 336326 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 youenn fablet 2018-03-22 16:40:53 PDT
Created attachment 336328 [details]
Patch for landing
Comment 9 youenn fablet 2018-03-22 16:46:47 PDT
Created attachment 336333 [details]
Patch
Comment 10 youenn fablet 2018-03-22 16:47:37 PDT
(In reply to youenn fablet from comment #9)
> Created attachment 336333 [details]
> Patch

Added Block_release in dealloc after discussing with Jer.
Comment 11 WebKit Commit Bot 2018-03-22 18:18:36 PDT
Comment on attachment 336333 [details]
Patch

Clearing flags on attachment: 336333

Committed r229876: <https://trac.webkit.org/changeset/229876>
Comment 12 WebKit Commit Bot 2018-03-22 18:18:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-03-22 18:19:23 PDT
<rdar://problem/38776207>
Comment 14 WebKit Commit Bot 2018-03-22 22:09:02 PDT
Re-opened since this is blocked by bug 183929
Comment 15 youenn fablet 2018-03-23 10:02:49 PDT
Created attachment 336382 [details]
Patch
Comment 16 WebKit Commit Bot 2018-03-23 11:20:51 PDT
Comment on attachment 336382 [details]
Patch

Clearing flags on attachment: 336382

Committed r229908: <https://trac.webkit.org/changeset/229908>
Comment 17 WebKit Commit Bot 2018-03-23 11:20:53 PDT
All reviewed patches have been landed.  Closing bug.