1257569 - replaceTrack does not update peer identity

Status: RESOLVED WORKSFORME

(Core :: WebRTC, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STR:
1. Open https://jsfiddle.net/rv4v8n6w/
2. Hit [Start!] and share camera+mic.
3. Hit [Replace!] and share same (or different if you have) camera+mic.

Expected result: SecurityError (because 2nd stream has explicit peerIdentity).

Actual result:

10 second recording finishes & plays (including both camera angles in the same stream if you picked two different cameras).

(pehrsons has a patch that refactors all the principal stuff for tracks and streams, and fixes this in bug 1208371, so marking as blocked on that).

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Actually, looks like the patch pehrsons gave me worked (turning the video black, not SecurityError as I said in comment 0). Will upload it here.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1257569 - Update sink identity after adding track. r?mt

Review commit: https://reviewboard.mozilla.org/r/40879/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40879/

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8731846 [details]
MozReview Request: Bug 1257569 - Update sink identity after adding track. r?mt

https://reviewboard.mozilla.org/r/40879/#review37409

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8731846 [details]
MozReview Request: Bug 1257569 - Update sink identity after adding track. r?mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40879/diff/1-2/

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Fixed a merge mistake. Also I notice the video goes black, but the audio keeps going. Something's missing.

Should also make a test out of the fiddle.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8731846 [details]
MozReview Request: Bug 1257569 - Update sink identity after adding track. r?mt

Couldn't figure out how to ask for review in again mozReview...

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8731846 [details]
MozReview Request: Bug 1257569 - Update sink identity after adding track. r?mt

https://reviewboard.mozilla.org/r/40879/#review37427

Yes, please try to test this somehow.

Comment 8

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Also I notice the video goes black, but the audio
> keeps going. Something's missing.

You only replace video?

Is audio expected to go silent if it's bundled with video that breaks the privacy constraint, even if itself does not?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

No, I'm just a doofus. I've updated the fiddle and it now turns silent. Thanks for catching that.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

BTW, when it goes silent I see spew of this:

[Child 97804] ###!!! ASSERTION: Unknown format: 'aFormat == AUDIO_FORMAT_S16 || aFormat == AUDIO_FORMAT_FLOAT32', file /Users/Jan/moz/mozilla-central/dom/media/AudioSampleFormat.h, line 247

Would be nice if our asserts printed what the value actually was (it's AUDIO_FORMAT_SILENCE).

Is this a bug that should be filed somewhere?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

> Would be nice if our asserts printed what the value actually was (it's
> AUDIO_FORMAT_SILENCE).
> 
> Is this a bug that should be filed somewhere?

Some of the assertion routines don't take format strings (NS_ASSERTION?)

Sounds like the assert should be updated though (and maybe the code)

Comment 12

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> BTW, when it goes silent I see spew of this:
> 
> [Child 97804] ###!!! ASSERTION: Unknown format: 'aFormat == AUDIO_FORMAT_S16
> || aFormat == AUDIO_FORMAT_FLOAT32', file
> /Users/Jan/moz/mozilla-central/dom/media/AudioSampleFormat.h, line 247
> 
> Would be nice if our asserts printed what the value actually was (it's
> AUDIO_FORMAT_SILENCE).
> 
> Is this a bug that should be filed somewhere?

Here's where the silence comes from: https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#1000

I also have a patch for that in the queue for bug 1208371 - though not published yet.

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: [From bug 1208371] Don't treat audio chunks as mutable in MediaPipeline

Like so, but this won't apply cleanly on m-c.

Comment 15

[Andreas Pehrson [:pehrsons]]

jib, FYI with bug 1208371 landed, following your STR, my recording gets black after hitting "Replace!". Not sure if it's exactly per spec but much better.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

That matches comment 1. I may have been mixing peer identity and cross origin restrictions. Maybe we can close this.

Comment 17

[Martin Thomson [:mt:]]

Great!