1129263 - Remove hints from DOMMediaStream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Andreas Pehrson [:pehrsons]]

This tracks removal of hints (SetHintContents, etc.) from DOMMediaStream and friends.

Comment 1

[Andreas Pehrson [:pehrsons]]

I've started looking at this. Will file additional bugs as needed.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add an event to MediaStreamListener for handling atomically added tracks

This is following the same idea as FinishAddStreams() in bug 1116925.

I'm adding a new callback to MediaStreamListener so that (mainly) DOMMediaStream knows when all the initial streams have been added (since they're added to MediaStreamGraph atomically with bug 1116925).

HTMLMediaElement will then not have to guess if all tracks have been added or not when playing a stream.

Comment 3

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

Comment on attachment 8560377 [details] [diff] [review]
Part 1. Add an event to MediaStreamListener for handling atomically added tracks

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

excellent!!!!!

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add an event to MediaStreamListener for handling atomically added tracks (carries r=roc)

I broke TracksCreatedRunnable::Run() out to DOMMediaStream::TracksCreated and also added DOMMediaStream::mTracksCreated to avoid a race where DOMMediaStream::OnTracksAvailable() could trigger the callback if called after the first track was added but before TracksCreatedRunnable was run. IMO not big enough a change to require a re-review. Carrying forward r=roc.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Remove hardcoded track ids from DOMMediaStream and let MediaInfo carry them instead

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Part 3. Use MediaInfo instead of mHasAudio/Video in media element

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Part 4. Remove DOMMediaStream::TrackTypeHints

Comment 8

[Andreas Pehrson [:pehrsons]]

I think these are pretty polished but I want to see how they perform on try before going for the r?.

I'll also note as a reminder to myself that I _think_ the AddTrack()s we do in MediaPipeline might cause intermittents with these patches unless we find a way to queue them like we do with MediaEngine/MediaManager in bug 1116925.

Comment 9

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

Comment on attachment 8561338 [details] [diff] [review]
Part 1. Add an event to MediaStreamListener for handling atomically added tracks (carries r=roc)

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

r+ if that syntax works... though even if it does I think it would be clearer to spell out the loop

::: dom/media/MediaStreamGraph.cpp
@@ +228,5 @@
>          aStream->mUpdateTracks.RemoveElementAt(i);
>        }
>      }
> +    if (notifiedTrackCreated) {
> +      for (uint32_t i = 0; i < aStream->mListeners.Length(); ++i) {

nit: size_t (or to be pedantic, array_type::index_type, but that's typedefed to size_t in nsTArray.h)

@@ +230,5 @@
>      }
> +    if (notifiedTrackCreated) {
> +      for (uint32_t i = 0; i < aStream->mListeners.Length(); ++i) {
> +        MediaStreamListener* l = aStream->mListeners[i];
> +        l->NotifyFinishedTrackCreation(this);

merge into one line unless that confuses the compiler

::: dom/media/TrackUnionStream.cpp
@@ +123,5 @@
>            mappedTracksWithMatchingInputTracks.AppendElement(true);
>          }
>        }
> +      if (trackAdded) {
> +        for (MediaStreamListener* l : mListeners) {

This syntax works?

Comment 10

[Andreas Pehrson [:pehrsons]]

(In reply to Randell Jesup [:jesup] from comment #9)
> Comment on attachment 8561338 [details] [diff] [review]
> Part 1. Add an event to MediaStreamListener for handling atomically added
> tracks (carries r=roc)
> 
> ::: dom/media/TrackUnionStream.cpp
> @@ +123,5 @@
> >            mappedTracksWithMatchingInputTracks.AppendElement(true);
> >          }
> >        }
> > +      if (trackAdded) {
> > +        for (MediaStreamListener* l : mListeners) {
> 
> This syntax works?

Yes. See bug 1126552.

Comment 11

[Andreas Pehrson [:pehrsons]]

I think I'll hold this off until renegotiations land, since it will touch MediaPipeline where remote tracks are created.

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add an event to MediaStreamListener for handling atomically added tracks (carries r=roc,jesup)

Rebased and comment 9 addressed.

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Put hardcoded numeric TrackIDs in MediaInfo instead of DOMMediaStream

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Part 3. Add tracks atomically when capturing from decoder

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Part 4. Use MediaInfo instead of mHasAudio/Video in media element

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Part 5. Remove DOMMediaStream::TrackTypeHints

Comment 17

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8568938 [details] [diff] [review]
Part 2. Put hardcoded numeric TrackIDs in MediaInfo instead of DOMMediaStream

This is preparation work for the later patches. Letting TrackInfo carry a numeric TrackID that we can use when capturing said track to a stream.

Comment 18

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8568939 [details] [diff] [review]
Part 3. Add tracks atomically when capturing from decoder

Adds tracks atomically for mozCaptureStream().

Comment 19

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8568940 [details] [diff] [review]
Part 4. Use MediaInfo instead of mHasAudio/Video in media element

Since we'll anyway need access to MediaInfo to know the TrackID of any future captured tracks (next patch), mHasAudio and mHasVideo will now be redundant. This patch removes them and uses MediaInfo for that purpose (HasAudio(), HasVideo()) instead.

Comment 20

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

Comment on attachment 8568938 [details] [diff] [review]
Part 2. Put hardcoded numeric TrackIDs in MediaInfo instead of DOMMediaStream

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

Currently, HTMLMediaElement::MetadataLoaded calls DOMMediaStream::SetHintContents so when metadataloaded fires, the media stream will contain preinitialized DOM tracks even if we haven't decoded anything. It looks to me like this patch will break that. Am I wrong?

Comment 21

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

What I expected to see in this bug was that we'd change SetHintContents to take a list of (track type, track ID) pairs, which would immediately construct DOM track objects that are later bound to actual tracks when those tracks become available through the MediaStreamGraph.

Comment 22

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 8568938 [details] [diff] [review]
> Part 2. Put hardcoded numeric TrackIDs in MediaInfo instead of DOMMediaStream
> 
> Review of attachment 8568938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Currently, HTMLMediaElement::MetadataLoaded calls
> DOMMediaStream::SetHintContents so when metadataloaded fires, the media
> stream will contain preinitialized DOM tracks even if we haven't decoded
> anything. It looks to me like this patch will break that. Am I wrong?

It will get fixed when the hints are removed in a later patch.

Comment 23

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> What I expected to see in this bug was that we'd change SetHintContents to
> take a list of (track type, track ID) pairs, which would immediately
> construct DOM track objects that are later bound to actual tracks when those
> tracks become available through the MediaStreamGraph.

With the ability to now add tracks atomically, DOMMediaStream doesn't need to know the specific tracks that are going to be added. We can CreateDOMTrack them to expose them to JS early when that's needed, like for captureStream(). Hints were mostly needed for the media element to know what to expect before changing ready state anyway, correct? That is solved by adding the tracks atomically. Further opinions? Something I missed? Would you still prefer hints to be around in the form of a list of tracks?

Comment 24

[Andreas Pehrson [:pehrsons]]

Attachment: Part 5. Add intial remote PeerConnection tracks atomically to SourceMediaStream

This adds remote PeerConnection tracks (from MediaPipeline) atomically.

It's assuming it's safe to queue tracks on the main thread when the stream is new. The factory will pass down information if the pipelines shall queue or not. If not they'll dispatch the adding to the MSG thread to figure out how long the first segment of null data should be, like before.

Comment 25

[Andreas Pehrson [:pehrsons]]

Attachment: Part 6. Remove DOMMediaStream::TrackTypeHints

This cleans out the hints, and also fixes the issue raised in comment 20.

Comment 26

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

This approach looks fine, thanks!!!

Comment 27

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

Comment on attachment 8569625 [details] [diff] [review]
Part 6. Remove DOMMediaStream::TrackTypeHints

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

::: dom/media/MediaRecorder.cpp
@@ +295,5 @@
>       : mSession(aSession) {}
>      virtual void NotifyTracksAvailable(DOMMediaStream* aStream)
>      {
> +      uint8_t trackTypes = 0;
> +      nsTArray<nsRefPtr<mozilla::dom::AudioStreamTrack> > audioTracks;

"> >" isn't needed anymore; >> instead

::: dom/media/MediaStreamGraph.cpp
@@ +2783,5 @@
>      } else {
>        mDriver = new SystemClockDriver(this);
>      }
>    } else {
> +    mDriver = new OfflineClockDriver(this, MEDIA_GRAPH_TARGET_PERIOD_MS);

Not sure why this was flagged as a change...

Comment 28

[Andreas Pehrson [:pehrsons]]

Attachment: Part 5. Add intial remote PeerConnection tracks atomically to SourceMediaStream (carries r=jesup)

Fixes a logic error of when tracks shall be queued and adds a logging entry. Carries forward r=jesup.

Comment 29

[Andreas Pehrson [:pehrsons]]

Attachment: Part 6. Remove DOMMediaStream::TrackTypeHints (carries r=roc,jesup)

Fixes per comment 27, carrying forward r=roc,jesup.

Comment 30

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8568938 [details] [diff] [review]
Part 2. Put hardcoded numeric TrackIDs in MediaInfo instead of DOMMediaStream

See comment 22 - and the "part 6" patch for the fix.

Comment 31

[Andreas Pehrson [:pehrsons]]

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

Comment 32

[Andreas Pehrson [:pehrsons]]

Attachment: Part 5. Add intial remote PeerConnection tracks atomically to SourceMediaStream (carries r=jesup)

Fixes a printf format that busted the build on some platforms. Carrying forward r=jesup.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7def66ed0a51

Comment 33

[Andreas Pehrson [:pehrsons]]

Last try looks good. Check in please :)

Comment 34

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d75111f24991
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a672c5cae9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bce8a3b3b11
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4c15cbc9d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f314a751ca7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f725bbaff1c

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d75111f24991
https://hg.mozilla.org/mozilla-central/rev/d7a672c5cae9
https://hg.mozilla.org/mozilla-central/rev/4bce8a3b3b11
https://hg.mozilla.org/mozilla-central/rev/8a4c15cbc9d7
https://hg.mozilla.org/mozilla-central/rev/8f314a751ca7
https://hg.mozilla.org/mozilla-central/rev/8f725bbaff1c

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Is this something we could uplift to Aurora?

Comment 38

[Andreas Pehrson [:pehrsons]]

I'm a bit skeptical with my changes to MediaPipeline probably having some dependency on the renegotiation work. I'll try a rebase though to see how hard it is.

Comment 39

[Andreas Pehrson [:pehrsons]]

Hep, for some reason I thought renegotiation was only in nightly but it appears to be in aurora as well - and these patches applied cleanly.

Comment 40

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8568936 [details] [diff] [review]
Part 1. Add an event to MediaStreamListener for handling atomically added tracks (carries r=roc,jesup)

Approval Request Comment
[Feature/regressing bug #]: bug 879717 (well, introduced the test failing in bug 1123950)
[User impact if declined]: Bug 1123950 might occur (video dimensions not set on "loadedmetadata" when playing a stream in a video element)
[Describe test coverage new/current, TreeHerder]: All tests involving DOMMediaStream exercises this code.
[Risks and why]: Low; fairly large patchset but goes cleanly on aurora and has extensive test coverage. 
[String/UUID change made/needed]: None.

Comment 41

[Andreas Pehrson [:pehrsons]]

The approval request applies to all patches on this bug.

Comment 42

[Andreas Pehrson [:pehrsons]]

Bug 1140089 will have to be uplifted together with this bug, should it get approval. I can make an approval request there too if needed.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #42)
> Bug 1140089 will have to be uplifted together with this bug, should it get
> approval. I can make an approval request there too if needed.

Please do :)

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d689b47ef57
https://hg.mozilla.org/releases/mozilla-aurora/rev/d02ba6daf0cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/fdddf7161b59
https://hg.mozilla.org/releases/mozilla-aurora/rev/682025ac0b1c
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f50ff0a0768
https://hg.mozilla.org/releases/mozilla-aurora/rev/44664728d0e1