Support RTCRtpSender.dtmf
Created attachment 375511 [details] Patch
<rdar://problem/53924833>
Attachment 375511 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
There is some uncertainty with regards to the tone dictionary member and whether init is mandatory. Chrome and Firefox currently are more lax than the spec. I filed issues in webrtc-pc. Current patch aligns with Chrome.
Created attachment 375512 [details] Patch
Attachment 375512 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375512&action=review > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:104 > + m_isPendingPlayoutTask = false; It is a bit confusing that you clear this here, set to true in startPlayingTone, and clear it again in onTonePlayed. It might be easier to follow if playNextTone returned a bool to signal whether or not a tone was played so you can only clear it here when a tone is not played. > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:117 > + auto currentTone = m_tones.substring(0, 1); > + m_tones = m_tones.substring(1); Nit: I think this would be clearer if you used m_tones.characterStartingAt() and m_tones.remove(0) since you only send one tone at a time. > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:144 > + m_backend = nullptr; Nit: should this also clear the timer? > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.cpp:88 > + callOnMainThread([this, weakThis = m_weakThis, tone = toWTFString(tone)] { > + if (!weakThis) > + return; > + if (m_onTonePlayed) > + m_onTonePlayed(tone); > + }); This is called on the signaling thread, is it safe to use a weak ptr?
rdar://problem/41461502
Thanks for the review. (In reply to Eric Carlson from comment #7) > Comment on attachment 375512 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375512&action=review > > > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:104 > > + m_isPendingPlayoutTask = false; > > It is a bit confusing that you clear this here, set to true in > startPlayingTone, and clear it again in onTonePlayed. It might be easier to > follow if playNextTone returned a bool to signal whether or not a tone was > played so you can only clear it here when a tone is not played. OK, I was trying to show we were setting to true/false in specific cases. I'll update it to make it clearer. > > > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:117 > > + auto currentTone = m_tones.substring(0, 1); > > + m_tones = m_tones.substring(1); > > Nit: I think this would be clearer if you used m_tones.characterStartingAt() > and m_tones.remove(0) since you only send one tone at a time. OK > > Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:144 > > + m_backend = nullptr; > > Nit: should this also clear the timer? Right. > > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.cpp:88 > > + callOnMainThread([this, weakThis = m_weakThis, tone = toWTFString(tone)] { > > + if (!weakThis) > > + return; > > + if (m_onTonePlayed) > > + m_onTonePlayed(tone); > > + }); > > This is called on the signaling thread, is it safe to use a weak ptr? This is thread safe as m_weakThis is thread safe refcounted and we are checking weakThis in the main thread.
Created attachment 375562 [details] Patch for landing
Attachment 375562 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375562 [details] Patch for landing Bug in the m_isPendingPlayoutTask refactoring
Created attachment 375577 [details] Patch for landing
Attachment 375577 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCDTMFSenderBackend.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375577 [details] Patch for landing Clearing flags on attachment: 375577 Committed r248282: <https://trac.webkit.org/changeset/248282>
All reviewed patches have been landed. Closing bug.
This breaks Linux 32-bit build : ../../Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp: In member function ‘WebCore::ExceptionOr<void> WebCore::RTCDTMFSender::insertDTMF(const WTF::String&, size_t, size_t)’: ../../Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:97:62: error: no matching function for call to ‘max(size_t&, const long unsigned int&)’ m_interToneGap = std::max(interToneGap, minInterToneGapMs); Either minInterToneGapMs must be a size_t, or a cast must be added. minToneDurationMs and maxToneDurationMs should perhaps be size_t too, event if there is no warning on the clampTo call.
(In reply to Loïc Yhuel from comment #17) > This breaks Linux 32-bit build : > ../../Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp: In member > function ‘WebCore::ExceptionOr<void> > WebCore::RTCDTMFSender::insertDTMF(const WTF::String&, size_t, size_t)’: > ../../Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:97:62: error: no > matching function for call to ‘max(size_t&, const long unsigned int&)’ > m_interToneGap = std::max(interToneGap, minInterToneGapMs); > > Either minInterToneGapMs must be a size_t, or a cast must be added. > minToneDurationMs and maxToneDurationMs should perhaps be size_t too, event > if there is no warning on the clampTo call. Right, it seems we should update min/max types. Do you want to provide a patch?
Created attachment 375624 [details] 32-bit build fix Here is a patch, generated manually since webkit-patch upload doesn't work (probably since it cannot reopen the bug).
Comment on attachment 375624 [details] 32-bit build fix Thanks!
(In reply to youenn fablet from comment #20) > Comment on attachment 375624 [details] > 32-bit build fix > > Thanks! The EWS doesn't want to process the patch since the bug is closed, so the commit queue won't take it. Should I submit a new bug ?
It broke 32bit linux build
(In reply to Loïc Yhuel from comment #21) > (In reply to youenn fablet from comment #20) > > Comment on attachment 375624 [details] > > 32-bit build fix > > > > Thanks! > > The EWS doesn't want to process the patch since the bug is closed, so the > commit queue won't take it. > Should I submit a new bug ? I reopened the bug. Let's try this way. Usually , new bugs are better indeed.
Let's try again and otherwise, let's land it in another bug.
Comment on attachment 375624 [details] 32-bit build fix Rejecting attachment 375624 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 375624, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/12871053
Hum, let's open a new bug then!