Bug 200431 – Support RTCRtpSender.dtmf

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Support RTCRtpSender.dtmf

[youenn fablet]

Created attachment 375511 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/53924833>

[EWS Watchlist]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 375512 [details]
Patch

[EWS Watchlist]

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.

[Eric Carlson]

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?

[Jon Lee]

rdar://problem/41461502

[youenn fablet]

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.

[youenn fablet]

Created attachment 375562 [details]
Patch for landing

[EWS Watchlist]

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.

[youenn fablet]

Comment on attachment 375562 [details]
Patch for landing

Bug in the m_isPendingPlayoutTask refactoring

[youenn fablet]

Created attachment 375577 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 375577 [details]
Patch for landing

Clearing flags on attachment: 375577

Committed r248282: <https://trac.webkit.org/changeset/248282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Loïc Yhuel]

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.

[youenn fablet]

(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?

[Loïc Yhuel]

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).

[youenn fablet]

Comment on attachment 375624 [details]
32-bit build fix

Thanks!

[Loïc Yhuel]

(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 ?

[youenn fablet]

It broke 32bit linux build

[youenn fablet]

(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.

[youenn fablet]

Let's try again and otherwise, let's land it in another bug.

[WebKit Commit Bot]

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

[youenn fablet]

Hum, let's open a new bug then!