Remove hints from DOMMediaStream

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

Trunk
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(6 attachments, 9 obsolete attachments)

13.18 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
11.55 KB, patch
jesup
: review+
roc
: review+
Details | Diff | Splinter Review
2.55 KB, patch
roc
: review+
jesup
: review+
Details | Diff | Splinter Review
11.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
41.90 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
23.72 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
This tracks removal of hints (SetHintContents, etc.) from DOMMediaStream and friends.
(Assignee)

Updated

4 years ago
Blocks: 1123950
(Assignee)

Comment 1

4 years ago
I've started looking at this. Will file additional bugs as needed.
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 1130290
(Assignee)

Comment 2

4 years ago
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.
Attachment #8560377 - Flags: review?(roc)
Attachment #8560377 - Flags: review?(rjesup)
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!!!!!
Attachment #8560377 - Flags: review?(roc) → review+
(Assignee)

Updated

4 years ago
See Also: → bug 1130299
(Assignee)

Comment 4

4 years ago
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.
Attachment #8560377 - Attachment is obsolete: true
Attachment #8560377 - Flags: review?(rjesup)
Attachment #8561338 - Flags: review?(rjesup)
(Assignee)

Comment 8

4 years ago
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 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?
Attachment #8561338 - Flags: review?(rjesup) → review+
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
I think I'll hold this off until renegotiations land, since it will touch MediaPipeline where remote tracks are created.
Depends on: 1017888
(Assignee)

Comment 13

4 years ago
Attachment #8561340 - Attachment is obsolete: true
Attachment #8561341 - Attachment is obsolete: true
Attachment #8561342 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
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.
Attachment #8568938 - Flags: review?(roc)
Attachment #8568938 - Flags: review?(rjesup)
(Assignee)

Comment 18

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

Adds tracks atomically for mozCaptureStream().
Attachment #8568939 - Flags: review?(roc)
Attachment #8568939 - Flags: review?(rjesup)
(Assignee)

Comment 19

4 years ago
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.
Attachment #8568940 - Flags: review?(roc)
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?
Attachment #8568938 - Flags: review?(roc)
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.
(Assignee)

Comment 22

4 years ago
(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.
(Assignee)

Comment 23

4 years ago
(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?
Flags: needinfo?(roc)
Flags: needinfo?(rjesup)
(Assignee)

Comment 24

4 years ago
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.
Attachment #8568942 - Attachment is obsolete: true
Attachment #8569624 - Flags: review?(rjesup)
(Assignee)

Comment 25

4 years ago
This cleans out the hints, and also fixes the issue raised in comment 20.
Attachment #8569625 - Flags: review?(roc)
Attachment #8569625 - Flags: review?(rjesup)
This approach looks fine, thanks!!!
Flags: needinfo?(roc)
Attachment #8568938 - Flags: review?(rjesup) → review+
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...
Attachment #8569625 - Flags: review?(rjesup) → review+
Attachment #8568939 - Flags: review?(rjesup) → review+
Attachment #8569624 - Flags: review?(rjesup) → review+
(Assignee)

Comment 28

4 years ago
Fixes a logic error of when tracks shall be queued and adds a logging entry. Carries forward r=jesup.
Attachment #8569624 - Attachment is obsolete: true
Attachment #8571172 - Flags: review+
(Assignee)

Comment 29

4 years ago
Fixes per comment 27, carrying forward r=roc,jesup.
Attachment #8569625 - Attachment is obsolete: true
Attachment #8571173 - Flags: review+
(Assignee)

Comment 30

4 years ago
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.
Attachment #8568938 - Flags: review?(roc)
(Assignee)

Comment 32

4 years ago
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
Attachment #8571172 - Attachment is obsolete: true
Attachment #8571233 - Flags: review+
(Assignee)

Comment 33

4 years ago
Last try looks good. Check in please :)
Flags: needinfo?(rjesup)
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 805717
Is this something we could uplift to Aurora?
Flags: needinfo?(pehrsons)
(Assignee)

Comment 38

4 years ago
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.
Flags: needinfo?(pehrsons)
(Assignee)

Comment 39

4 years ago
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.
(Assignee)

Comment 40

4 years ago
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.
Attachment #8568936 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 41

4 years ago
The approval request applies to all patches on this bug.

Updated

4 years ago
Depends on: 1140089
(Assignee)

Comment 42

4 years ago
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.
(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 :)
Attachment #8568936 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: --- → affected
(Assignee)

Updated

4 years ago
Depends on: 1116925
Blocks: 1073406
No longer blocks: 1073406
You need to log in before you can comment on or make changes to this bug.