1103848 - Make sure captured media elements report their tracks as soon as possible

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Andreas Pehrson [:pehrsons]]

From findings in bug 879717,

A captured media element normally reports its tracks as they become available.
However, for another media element playing the stream of the captured element it is important to know if the stream will contain a video track as soon as possible.

We can use hints to make sure the tracks are reported as soon as possible. We should also report video first so the playing media element does not do any assumptions based on audio only playout.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add video track before audio track for media decoder output streams

From bug 879717, identical to attachment 8520614 [details] [diff] [review] which already has r+ from roc.

Adding video tracks before audio tracks when sending stream data from decoder.

I'm not sure this is actually needed if we setup the hints properly. Will test.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Hint existing output streams as tracks become known

An output stream already gets hinted of which track types it consists of, if the tracks are known at output stream creation time.

This patch also sets hints on all output streams that exist when tracks become known.

I'll leave the other patch out for now. Should not be relevant anymore.

Comment 3

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8527537 [details] [diff] [review]
Hint existing output streams as tracks become known

Review of attachment 8527537 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/DOMMediaStream.cpp
@@ -53,5 @@
>          track = stream->BindDOMTrack(mID, mType);
>          if (!track) {
>            stream->CreateDOMTrack(mID, mType);
>            track = stream->BindDOMTrack(mID, mType);
> -          stream->NotifyMediaStreamTrackCreated(track);

Previous behavior was to notify that a track was created only if it hadn't been previously created. I'm just assuming that we always want to notify at time of BindDOMTrack() here. Otherwise there is no notify of a created track when it gets created through SetHintContents().

Comment 4

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8527537 [details] [diff] [review]
Hint existing output streams as tracks become known

Review of attachment 8527537 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +2907,5 @@
> +  uint8_t hints = (mHasAudio ? DOMMediaStream::HINT_CONTENTS_AUDIO : 0) |
> +                  (mHasVideo ? DOMMediaStream::HINT_CONTENTS_VIDEO : 0);
> +  for (uint32_t i = 0; i < mOutputStreams.Length(); ++i) {
> +    OutputMediaStream* out = &mOutputStreams[i];
> +    if (out->mStream->GetHintContents() != hints) {

Also, this check is not needed anymore. I fixed it properly in DOMMediaStream instead.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Hint existing output streams as tracks become known

Fix per comment 4.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Hint existing output streams as tracks become known

That went a bit too fast.. said the compiler.

Comment 7

[Andreas Pehrson [:pehrsons]]

Forgot to mention, a try is under way: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=096c2e0181bb

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Hint existing output streams as tracks become known

A small change to the previous patch.

Now when a track gets created, we update the hint contents to also reflect reality.

So that a SetHintContents() call that comes after a track has been created for another reason does not create an extra track.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Make track ID constants consistent across MediaDecoder and webrtc

Existing behavior that could go wrong when capturing a media element:
1. SetHintContents creates an audio track with ID 1 (kAudioTrack in MediaEngine.h). (DOMMediaStream shouldn't depend on internals of MediaEngine like this)
2. A TrackChange comes in and tries to bind an audio track with ID 2 (TRACK_AUDIO from MediaDecoderStateMachine.cpp).
3. Binding fails because track has not been created.
4. An audio track with ID 2 gets created and bound. There are now two audio tracks, but should only be one.

This patch makes the IDs consistent for both webrtc uses and MediaDecoder uses. IMO this should be fixed properly when streams are fixed to support more than one audio and video track.

Comment 10

[Andreas Pehrson [:pehrsons]]

try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c8926719f5ce

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> 1. SetHintContents creates an audio track with ID 1 (kAudioTrack in
> MediaEngine.h). (DOMMediaStream shouldn't depend on internals of MediaEngine
> like this)

Can you explain how webrtc gets its hands on a stream captured from a media element?

Comment 12

[Andreas Pehrson [:pehrsons]]

(In reply to JW Wang [:jwwang] from comment #11)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> > 1. SetHintContents creates an audio track with ID 1 (kAudioTrack in
> > MediaEngine.h). (DOMMediaStream shouldn't depend on internals of MediaEngine
> > like this)
> 
> Can you explain how webrtc gets its hands on a stream captured from a media
> element?

Webrtc doesn't get its hands on it, but DOMMediaStream::SetHintContents uses the constants from webrtc (MediaEngine.h) when creating tracks. It could just as well use the constants from MediaDecoder instead, but then trouble could happen when DOMMediaStream::SetHintContents is called from webrtc, since that method is called from both sides.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Can you paste the call stack of how DOMMediaStream::SetHintContents is called from MediaEngine.h?

Comment 14

[Andreas Pehrson [:pehrsons]]

Here is a call stack from running `./mach mochitest dom/media/test/test_mediatrack_events.html`:

  * frame #0: 0x00000001038ded75 XUL`mozilla::DOMMediaStream::SetHintContents(this=0x0000000126e48ad0, aHintContents='\x03') + 21 at DOMMediaStream.cpp:329
    frame #1: 0x00000001038defda XUL`mozilla::DOMMediaStream::InitTrackUnionStream(this=0x0000000126e48ad0, aWindow=<unavailable>, aHintContents='\x03') + 122 at DOMMediaStream.cpp:239
    frame #2: 0x0000000103949015 XUL`mozilla::nsDOMUserMediaStream::CreateTrackUnionStream(aWindow=0x000000012b059420, aListener=<unavailable>, aAudioSource=<unavailable>, aVideoSource=<unavailable>) + 357 at MediaManager.cpp:563
    frame #3: 0x000000010394824d XUL`mozilla::GetUserMediaStreamRunnable::Run(this=0x0000000128fd23a0) + 1229 at MediaManager.cpp:841
    frame #4: 0x0000000101ddf1bf XUL`nsThread::ProcessNextEvent(this=0x0000000100331310, aMayWait=<unavailable>, aResult=0x00007fff5fbfcce7) + 1487 at nsThread.cpp:830
    frame #5: 0x0000000101e0c7df XUL`NS_ProcessPendingEvents(aThread=<unavailable>, aTimeout=20) + 79 at nsThreadUtils.cpp:207
(...)

Comment 15

[Andreas Pehrson [:pehrsons]]

(In reply to JW Wang [:jwwang] from comment #13)
> Can you paste the call stack of how DOMMediaStream::SetHintContents is
> called from MediaEngine.h?

Oh, from MediaEngine.h?

That's just where the constants are defined; the above callstack is just showing SetHintContents being used from the webrtc side.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8528201 [details] [diff] [review]
Part 2. Make track ID constants consistent across MediaDecoder and webrtc

Review of attachment 8528201 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +355,5 @@
>    aOutput->AppendFrame(image.forget(), duration, aIntrinsicSize);
>  }
>  
> +// XXX See bug 1103848: Keep these track IDs consistent with the constants in
> +// dom/media/webrtc/MediaEngine.h

Add a comment to indicate these constants are also used in DOMMediaStream.cpp

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Make track ID constants consistent across MediaDecoder and webrtc (carries f=jwwang)

Per comment 16.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8528281 [details] [diff] [review]
Part 2. Make track ID constants consistent across MediaDecoder and webrtc (carries f=jwwang)

Review of attachment 8528281 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +356,5 @@
>  }
>  
> +// XXX See bug 1103848: Keep these track IDs consistent with the constants in
> +// dom/media/webrtc/MediaEngine.h. dom/media/DOMMediaStream.cpp uses the
> +// constants from MediaEngine.h but may also receive tracks with the IDs below.

I think we should move these to dom/media/DOMMediaStream.h.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The long-term solution is to expand the hints from a simple bit mask to a list of (track ID, type). That will support pre-creating multiple tracks of the same type and avoid having to hardcode track IDs here.

Comment 20

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Use a single source for audio/video track ID constants

Comment 21

[Andreas Pehrson [:pehrsons]]

tries:
https://tbpl.mozilla.org/?tree=Try&rev=dde9ebcf4109
https://tbpl.mozilla.org/?tree=Try&rev=a22e85df7cad

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/947dce2c43a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d512a56abc5

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/947dce2c43a6
https://hg.mozilla.org/mozilla-central/rev/8d512a56abc5

Comment 24

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8528197 [details] [diff] [review]
Part 1. Hint existing output streams as tracks become known

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Blocking uplift of bug 879717, see bug 879717 comment 200 for its approval request comment.
[Describe test coverage new/current, TBPL]: Landed on m-c in 37. Covered by most MediaStream tests.
[Risks and why]: low, doesn't change any logic.
[String/UUID change made/needed]: None

This request applies to all patches on this bug.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8528197 [details] [diff] [review]
Part 1. Hint existing output streams as tracks become known

Sorry but same rational as in bug 879717.