Bug 219148

Summary: Add support for RTCRtpScriptTransform
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, kangil.han, kondapallykalyan, philipj, ryuan.choi, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch for landing none

Description youenn fablet 2020-11-19 00:27:12 PST 
Add support for RTCRtpScriptTransform
Comment 1 youenn fablet 2020-11-19 02:41:18 PST 
Created attachment 414553 [details]
Patch
Comment 2 youenn fablet 2020-11-19 02:49:25 PST 
Created attachment 414556 [details]
Patch
Comment 3 youenn fablet 2020-11-19 02:51:40 PST 
Created attachment 414557 [details]
Patch
Comment 4 youenn fablet 2020-11-19 03:01:54 PST 
Created attachment 414559 [details]
Patch
Comment 5 youenn fablet 2020-11-19 03:12:03 PST 
Created attachment 414560 [details]
Patch
Comment 6 youenn fablet 2020-11-19 04:00:40 PST 
Created attachment 414564 [details]
Patch
Comment 7 Eric Carlson 2020-11-19 13:02:23 PST 
Comment on attachment 414564 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpScriptTransform.cpp:117
> +        m_backend = makeRef(backend);

You only set `m_backend` if `setupTransformer` fails?

> Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:174
> +            auto value = isAudio ? toJS(&globalObject, &globalObject, RTCEncodedAudioFrame::create(WTFMove(frame))) : toJS(&globalObject, &globalObject, RTCEncodedAudioFrame::create(WTFMove(frame)));

Don’t you want to create a Video frame when `isAudio` is false?
Comment 8 youenn fablet 2020-11-20 00:35:26 PST 
(In reply to Eric Carlson from comment #7)
> Comment on attachment 414564 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414564&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpScriptTransform.cpp:117
> > +        m_backend = makeRef(backend);
> 
> You only set `m_backend` if `setupTransformer` fails?

Yes, it means the transformer is not yet there but will be soon.
So we keep a ref to m_backend so that we can call setupTransformer at the time the transformer is hooked to its transform.

> > Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:174
> > +            auto value = isAudio ? toJS(&globalObject, &globalObject, RTCEncodedAudioFrame::create(WTFMove(frame))) : toJS(&globalObject, &globalObject, RTCEncodedAudioFrame::create(WTFMove(frame)));
> 
> Don’t you want to create a Video frame when `isAudio` is false?

Good catch, will add a test for it.
Comment 9 youenn fablet 2020-11-20 00:50:38 PST 
Created attachment 414659 [details]
Patch for landing
Comment 10 EWS 2020-11-20 04:25:55 PST 
Committed r270107: <https://trac.webkit.org/changeset/270107>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414659 [details].
Comment 11 Radar WebKit Bug Importer 2020-11-20 04:26:16 PST 
<rdar://problem/71623266>