Bug 191490

Summary:

RealtimeOutgoing A/V sources should observe their sources only if having a sink

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebRTC

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric.carlson, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

youenn fablet

Reported 2018-11-09 15:12:00 PST

This is useless otherwise

Attachments
Patch (18.84 KB, patch) 2018-11-09 15:37 PST, youenn fablet	no flags	Details Formatted Diff Diff
Patch (19.91 KB, patch) 2018-11-09 16:42 PST, youenn fablet	no flags	Details Formatted Diff Diff
Patch (20.19 KB, patch) 2018-11-09 20:46 PST, youenn fablet	no flags	Details Formatted Diff Diff
Patch (21.02 KB, patch) 2018-11-09 21:21 PST, youenn fablet	no flags	Details Formatted Diff Diff
Patch for landing (22.01 KB, patch) 2018-11-12 11:00 PST, youenn fablet	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2018-11-09 15:37:55 PST

Created attachment 354395 [details] Patch

youenn fablet

Comment 2 2018-11-09 16:42:16 PST

Created attachment 354413 [details] Patch

youenn fablet

Comment 3 2018-11-09 20:46:27 PST

Created attachment 354434 [details] Patch

youenn fablet

Comment 4 2018-11-09 21:21:04 PST

Created attachment 354438 [details] Patch

Eric Carlson

Comment 5 2018-11-12 08:29:15 PST

Comment on attachment 354438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354438&action=review > Source/WebCore/platform/mediastream/RealtimeOutgoingAudioSource.cpp:65 > + observeSource(); As we discussed, this shouldn't be necessary because you also call it in AddSink. > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:73 > + observeSource(); Ditto. > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:81 > m_videoSource->removeObserver(*this); Nit: why does RealtimeOutgoingAudioSource have unobserveSource and RealtimeOutgoingVideoSource just calls removeObserver?

youenn fablet

Comment 6 2018-11-12 10:27:50 PST

Thanks for the review. (In reply to Eric Carlson from comment #5) > Comment on attachment 354438 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354438&action=review > > > Source/WebCore/platform/mediastream/RealtimeOutgoingAudioSource.cpp:65 > > + observeSource(); > > As we discussed, this shouldn't be necessary because you also call it in > AddSink. Yes, I will lock the sinks so that we observe/unobserve appropriately. > > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:73 > > + observeSource(); > > Ditto. > > > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:81 > > m_videoSource->removeObserver(*this); > > Nit: why does RealtimeOutgoingAudioSource have unobserveSource and > RealtimeOutgoingVideoSource just calls removeObserver? I will add unobserveSource.

youenn fablet

Comment 7 2018-11-12 11:00:36 PST

Created attachment 354571 [details] Patch for landing

WebKit Commit Bot

Comment 8 2018-11-12 11:49:20 PST

Comment on attachment 354571 [details] Patch for landing Clearing flags on attachment: 354571 Committed r238102: <https://trac.webkit.org/changeset/238102>

WebKit Commit Bot

Comment 9 2018-11-12 11:49:22 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 10 2018-11-12 11:50:38 PST

<rdar://problem/45998054>

Note You need to log in before you can comment on or make changes to this bug.