<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189#c10">Comment # 10</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: Add RTCRtpTransceiver interface and RTCPeerConnection.addTransceiver()"
href="https://bugs.webkit.org/show_bug.cgi?id=158189">bug 158189</a>
from <span class="vcard"><a class="email" href="mailto:adam.bergkvist@ericsson.com" title="Adam Bergkvist <adam.bergkvist@ericsson.com>"> <span class="fn">Adam Bergkvist</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=158189#c7">comment #7</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=280093&action=diff" name="attach_280093" title="Proposed patch">attachment 280093</a> <a href="attachment.cgi?id=280093&action=edit" title="Proposed patch">[details]</a></span>
> Proposed patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=280093&action=review">https://bugs.webkit.org/attachment.cgi?id=280093&action=review</a>
>
> Noticed a few more things while re-looking at this that we could refine
> later.
>
> >>> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:69
> >>> + [StrictTypeChecking, RaisesException] RTCRtpTransceiver addTransceiver(DOMString kind, optional Dictionary init);
> >>
> >> New code should never use Dictionary. These dictionaries should defined.
> >>
> >> dictionary RTCRtpTransceiverInit {
> >> boolean send = true;
> >> boolean receive = true;
> >> sequence<MediaStream> streams;
> >> sequence<RTCRtpEncodingParameters> sendEncodings;
> >> };
> >>
> >> If the bindings generator can’t handle this dictionary, we should fix it so it can handle it. Every single time we instead use the deprecated Dictionary, we add code we will need to later delete!
> >
> > Using dictionary from latest published editor's draft [1]. Not all members are supported yet. Should I add a FIXME+bug# for that?
> >
> > [1] <a href="https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtcrtptransceiverinit">https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtcrtptransceiverinit</a>
>
> I don’t think we need a FIXME about each piece that is missing as we
> implement something new like this. Eventually it might have some value. but
> during development it’s probably OK to just do things a bit at a time.
>
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:58
> > + if (track)
> > + setTrack(WTFMove(track));
>
> Not sure the if here is valuable.</span >
Let's let setTrack() handle track being null
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:47
> > virtual ~RTCRtpSenderReceiverBase() { }
>
> It’s probably more elegant to write "= default" here instead of writing out
> an empty body.</span >
Fixed.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCRtpSenderReceiverBase.h:53
> > + RTCRtpSenderReceiverBase()
> > + { }
>
> It’s probably more elegant to write "= default" here instead of writing out
> an empty body.</span >
Fixed.
<span class="quote">> >>> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:98
> >>> +}
> >>
> >> Since bindings create functions like this for all enumerations, we should avoid writing this code ourselves if possible. What makes this necessary?
> >
> > Removed parseDirectionString() as part of moving to properly defined dictionary in idl. directionString() is used when the direction value is added to a PeerMediaDescription (which represents direction as a string). I guess there's no nice way to use the generated version from the binding?
>
> Yes, at the moment there is no way to convert an enumeration into the string
> from the bindings, but if this pattern keeps recurring, I suppose we can add
> a way.</span >
(In reply to <a href="show_bug.cgi?id=158189#c8">comment #8</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=280145&action=diff" name="attach_280145" title="Updated patch">attachment 280145</a> <a href="attachment.cgi?id=280145&action=edit" title="Updated patch">[details]</a></span>
> Updated patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=280145&action=review">https://bugs.webkit.org/attachment.cgi?id=280145&action=review</a>
>
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:195
> > + transceiver->setDirection(static_cast<RTCRtpTransceiver::Direction>(init.direction));
>
> This typecast should not be needed. It’s currently only needed because we
> are defining the enum twice (see my other comments).</span >
Agreed. See comments below.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:67
> > Vector<RefPtr<RTCRtpSender>> getSenders() const override { return m_senderSet; }
> > Vector<RefPtr<RTCRtpReceiver>> getReceivers() const { return m_receiverSet; }
>
> It’s expensive to have functions that return an existing Vector by copying
> it. We should not do that unless we have to.</span >
The separate fields for storing senders and receivers will be removed shortly since an RTCRtpTransceiver wraps these. They're kept for now to keep the size of this change smaller. The new versions will look into the m_transceiverSet to create the sender and receiver lists so there might be some optimizations to do there as well. These normally contain only a few elements. Sending and receiving 10 streams is quite a lot.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:78
> > + enum class RtpTransceiverDirection {
> > + Sendrecv,
> > + Sendonly,
> > + Recvonly,
> > + Inactive
> > + };
>
> Instead of re-defining this here, we should write this:
>
> using RtpTransceiverDirection = RTCRtpTransceiver::Direction;</span >
This would be much nicer. However, applying this change makes the bindings generator generate duplicate conversion functions (jsStringWithCache, parse, convert) for the RTCRtpTransceiverDirection enum with the same C++ enum. I'll leave as is and file a new bug.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:82
> > + struct RtpTransceiverInit {
> > + RtpTransceiverDirection direction;
> > + };
>
> Eventually, we probably also want to define this in RTCRtpTransceiver and
> use "using" to pull it in here with the appropriate name.
>
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:125
> > +enum RTCRtpTransceiverDirection {
> > + "sendrecv",
> > + "sendonly",
> > + "recvonly",
> > + "inactive"
> > +};
>
> Eventually we will make the bindings script smarter so we don’t have to
> repeat this in multiple IDL files. In the mean time we might want to comment
> so anyone modifying one knows they should modify the other.</span >
Agreed. Adding comments.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCRtpSender.h:60
> > + bool isStopped() const { return m_client == nullptr; }
>
> WebKit coding style says we write this !m_client rather than m_client ==
> nullptr.</span >
Fixed.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:80
> > + return emptyString();
>
> Maybe return inactiveString() here instead of emptyString()?</span >
Fixed.
<span class="quote">> > Source/WebCore/Modules/mediastream/RTCRtpTransceiver.h:52
> > + enum class Direction {
> > + Sendrecv,
> > + Sendonly,
> > + Recvonly,
> > + Inactive
> > + };
>
> I find short enumerations like this really nice when writing out all on one
> line.</span >
The direction enums will likely never be extended so let's make them a one-liner.
<span class="quote">> > Source/WebCore/platform/mock/MockRealtimeAudioSource.cpp:50
> > + Ref<MockRealtimeAudioSource> source = adoptRef(*new MockRealtimeAudioSource(name));
>
> I think auto works better here than writing the type a second time. I’d also
> omit the empty line below.</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:59
> > + Ref<MockRealtimeVideoSource> source = adoptRef(*new MockRealtimeVideoSource(name));
>
> I think auto works better here than writing the type a second time. I’d also
> omit the empty line below.</span >
Fixed.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>