Bug 222982 - Update RTCRtpScriptTransform to the latest version of the spec
Summary: Update RTCRtpScriptTransform to the latest version of the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 223142
Blocks:
  Show dependency tree / graph
 
Reported: 2021-03-09 09:20 PST by youenn fablet
Modified: 2021-03-13 03:12 PST (History)
26 users (show)

See Also:

Attachments
Patch (65.84 KB, patch)
2021-03-09 09:32 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (72.64 KB, patch)
2021-03-10 05:32 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch for landing (73.75 KB, patch)
2021-03-12 06:43 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Win fix try (74.59 KB, patch)
2021-03-12 08:09 PST, youenn fablet 		no flags Details | Formatted Diff | Diff
Patch (76.64 KB, patch)
2021-03-12 13:48 PST, Fujii Hironori 		ews-feeder: commit-queue-
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 2021-03-09 09:20:11 PST 
Update RTCRtpScriptTransform to the latest version of the spec
Comment 1 youenn fablet 2021-03-09 09:32:51 PST 
Created attachment 422716 [details]
Patch
Comment 2 youenn fablet 2021-03-10 05:32:14 PST 
Created attachment 422821 [details]
Patch
Comment 3 youenn fablet 2021-03-10 10:06:48 PST 
Comment on attachment 422821 [details]
Patch

Wincairo might be a unified build issue
Comment 4 Eric Carlson 2021-03-11 08:59:35 PST 
Comment on attachment 422821 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Add support for options parameter provided in RTCRtpScriptTrtansform constructor.

s/RTCRtpScriptTrtansform/RTCRtpScriptTransform/

> Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:55
> +    auto transformer = adoptRef(*new RTCRtpScriptTransformer(context, WTFMove(options), WTFMove(port), readable.releaseReturnValue(), WTFMove(readableSource)));

Is there any reason to not have the constructor get the SimpleReadableStreamSource from the ReadableStream?

> Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:104
> +            return writable;

I thought this was wrong at first because of the variable name, something like `writableOrException` might make it clearer.
Comment 5 youenn fablet 2021-03-12 02:19:39 PST 
(In reply to Eric Carlson from comment #4)
> Comment on attachment 422821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422821&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Add support for options parameter provided in RTCRtpScriptTrtansform constructor.
> 
> s/RTCRtpScriptTrtansform/RTCRtpScriptTransform/

OK

> > Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:55
> > +    auto transformer = adoptRef(*new RTCRtpScriptTransformer(context, WTFMove(options), WTFMove(port), readable.releaseReturnValue(), WTFMove(readableSource)));
> 
> Is there any reason to not have the constructor get the
> SimpleReadableStreamSource from the ReadableStream?

ReadableStream can take any ReadableStreamSource but we want to keep the fact it is a SimpleReadableStreamSource.

> > Source/WebCore/Modules/mediastream/RTCRtpScriptTransformer.cpp:104
> > +            return writable;
> 
> I thought this was wrong at first because of the variable name, something
> like `writableOrException` might make it clearer.

OK
Comment 6 youenn fablet 2021-03-12 06:43:13 PST 
Created attachment 423043 [details]
Patch for landing
Comment 7 youenn fablet 2021-03-12 08:09:22 PST 
Created attachment 423052 [details]
Win fix try
Comment 8 youenn fablet 2021-03-12 09:55:38 PST 
Don, Hironori-san, can you help me with wincairo failure?
Might be a unified build issue.
Comment 9 Fujii Hironori 2021-03-12 12:19:43 PST 
Thank you for letting me know that. It looks a simple problem. Will check.
Comment 10 Fujii Hironori 2021-03-12 13:48:13 PST 
Created attachment 423080 [details]
Patch
Comment 11 Fujii Hironori 2021-03-12 13:55:34 PST 
It turned out this isn't a simple problem. I'll look into into it again next week.
I added some header inclusions to WebGLRenderingContextBase.h, and fixed recursive inclusion of WebGLTransformFeedback.h.
This patch can compile for WinCairo.
Hmm, but, it can't compile for other ports.
Comment 12 EWS 2021-03-13 02:29:01 PST 
commit-queue failed to commit attachment 423052 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 13 EWS 2021-03-13 02:51:18 PST 
commit-queue failed to commit attachment 423052 [details] to WebKit repository. To retry, please set cq+ flag again.
Comment 14 EWS 2021-03-13 03:11:32 PST 
Committed r274385: <https://commits.webkit.org/r274385>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423052 [details].
Comment 15 Radar WebKit Bug Importer 2021-03-13 03:12:16 PST 
<rdar://problem/75392479>