Closed Bug 1266646 Opened 8 years ago Closed 8 years ago

Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamDirectTrackListener.

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ctai, Assigned: ctai)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Once bug 1201363 landed, the TrackUnionStream will not copy track data from input in video case. So the NotifyQueuedTrackChange will not be called in such case. And that means HTMLMediaElement::StreamSizeListerner will not work.
Assignee: nobody → ctai
Summary: Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamDirectListener. → Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamDirectTrackListener.
I would like to make HTMLMediaElement::StreamSizeListerner to inherit MediaStreamDirectTrackListener. The reason to change to MSDTL is:
1. We don't need to listen every tracks in the SourceMediaStream, although that will only contain two tracks in any case so far.
2. Lack of defer mechanism for MediaStreamDirectListener. Right now, the MSDL will be added into DOMMediaStream. But the SourceMediaStream might not exist when the listener is added.

So I would like to use MSDTL. But that also lead to another problem. Right now, the MSDTL only is allowed to added into audio track. I need to relax the constraint. My guess for the reason why roc propose this kind of constraint is the video data could go through the VideoFrameContainer. That is also the original design of my patch in bug 1201363.

But this kind of design will cause the regression of bug 1240478. Because it is bound with VideoFrameContainer too closely, so the behavior is depended on VideoFrameContainer. That cause a problem that the size data will be notified when the VideoFrameContainer is added. For example, two MediaElement, src of v1 is from local video file. src of v2 is from v1.captureStream. Because the VideoFrameContainer will not be added until v2 is played, the size of v2 will not change until v2 is played even v1 played. Bug 1240478 will happen again. 

Another question will be which video track will be bound. Right now, the size will be set once and only once. So the track automatically changing stuff should not need to be considered.

jesup, WDYT?
Flags: needinfo?(rjesup)
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #1)
> Another question will be which video track will be bound. Right now, the
> size will be set once and only once. So the track automatically changing
> stuff should not need to be considered.

What when you start playing a MediaStream with two video tracks?

If we had video track selection implemented, this would be easy.
I'll need to think about this more. ...  Keeping needinfo
Rank: 23
Flags: needinfo?(rjesup)
Priority: -- → P2
Looks like jesup remove the ni flag accidentally. Adding back the ni flag.

(In reply to Randell Jesup [:jesup] from comment #3)
> I'll need to think about this more. ...  Keeping needinfo
Flags: needinfo?(rjesup)
After checking more on the current mSelectedIndex implementation in Gecko, I would do below approach.
Some analysis first. Right now, every VideoTrack::mSelected should already assigned correct value.
There are two case of VideoTrack, the original HTMLMediaElement and from MediaStream. That should already cover all situation.
In HTMLMediaElement case, |VideoTrack::SetEnabledInternal| is called in |MediaDecoder::ConstructMediaTracks|. The value of VideoTrack::mSelected is reflected the value of TrackInfo::mEnabled. Also the HTMLMediaElement case no need to consider multiple case so far.
In MediaStream case, |VideoTrack::SetEnabledInternal| is called in |HTMLMediaElement::ConstructMediaTracks| <- |HTMLMediaElement::SetupSrcMediaStreamPlayback|. |HTMLMediaElement::ConstructMediaTracks| will set the first enabled VideoStreamTrack as selected. The order of VideoStreamTracks is depended on DOMMediaStream::mTracks. If media resource does not indicate a particular set of video tracks to enable, the one that is listed first in the element's videoTracks object must be selected.

So I will append the MediaStreamDirectTrackListener to the VideoTrackList::mSelectedIndex th VideoTrack. That means:
1. VideoTrack might hold a pointer of VideoStreamTracks.
2. Allow MediaStreamDirectTrackListener to append to VideoTrack.
3. Add the MediaStreamDirectTrackListener in OnTrackAvailable callback.
4. Remove the MediaStreamDirectTrackListener once the stream size set and in desctuctor.

HDYT?
Flags: needinfo?(pehrsons)
Sounds reasonable
Flags: needinfo?(rjesup)
I think you can skip nr 2 since VideoTrack is also used for non-VideoStreamTracks. Should be simple enough to look up anyway. Especially if you do nr 1 (not sure if I prefer that over just looking the track up in the mSrcStream).

The rest sounds good.
Flags: needinfo?(pehrsons)
Attachment #8746919 - Flags: review?(pehrsons)
Attachment #8746920 - Flags: review?(pehrsons)
Comment on attachment 8746919 [details] [diff] [review]
Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Cancel review, I think I might not use OnTracksAvailableCallback.
Attachment #8746919 - Flags: review?(rjesup)
Attachment #8746919 - Flags: review?(pehrsons)
I am thinking, should we really need two class DOMMediaStream::OnTracksAvailableCallback and DOMMediaStream::TrackListener? Maybe we can combine them.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #10)
> Comment on attachment 8746919 [details] [diff] [review]
> Change HTMLMediaElement::StreamSizeListerner to inherit
> MediaStreamTrackDirectListener.
> 
> Cancel review, I think I might not use OnTracksAvailableCallback.
Attachment #8746920 - Flags: review?(rjesup) → review+
Attachment #8746920 - Flags: review?(pehrsons) → review+
After double check the code base:
See https://dxr.mozilla.org/mozilla-central/search?q=NotifyFinishedTrackCreation&redirect=false&case=false
I do think OnTracksAvailableCallback is redundant after TrackListener is introduced.


(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #11)
> I am thinking, should we really need two class
> DOMMediaStream::OnTracksAvailableCallback and DOMMediaStream::TrackListener?
> Maybe we can combine them.
> 
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #10)
> > Comment on attachment 8746919 [details] [diff] [review]
> > Change HTMLMediaElement::StreamSizeListerner to inherit
> > MediaStreamTrackDirectListener.
> > 
> > Cancel review, I think I might not use OnTracksAvailableCallback.
Well, wrong link.

Looks like this link break the definition "Notify that all new tracks this iteration have been created."
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#2059
Should be fixed someday.


(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #12)
> After double check the code base:
> See
> https://dxr.mozilla.org/mozilla-central/
> search?q=NotifyFinishedTrackCreation&redirect=false&case=false
> I do think OnTracksAvailableCallback is redundant after TrackListener is
> introduced.
> 
> 
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #11)
> > I am thinking, should we really need two class
> > DOMMediaStream::OnTracksAvailableCallback and DOMMediaStream::TrackListener?
> > Maybe we can combine them.
> > 
> > (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #10)
> > > Comment on attachment 8746919 [details] [diff] [review]
> > > Change HTMLMediaElement::StreamSizeListerner to inherit
> > > MediaStreamTrackDirectListener.
> > > 
> > > Cancel review, I think I might not use OnTracksAvailableCallback.
I am curious why roc use nsAutoPtr in the nsTArray. And in bug 866514 it point out there is a memory management issue. I think I should not combine OnTracksAvailableCallback with TrackListener until WebRTC code participated in cycle collection. :( 

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #13)
> Well, wrong link.
> 
> Looks like this link break the definition "Notify that all new tracks this
> iteration have been created."
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamGraph.
> cpp#2059
> Should be fixed someday.
> 
> 
> (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #12)
> > After double check the code base:
> > See
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=NotifyFinishedTrackCreation&redirect=false&case=false
> > I do think OnTracksAvailableCallback is redundant after TrackListener is
> > introduced.
> > 
> > 
> > (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #11)
> > > I am thinking, should we really need two class
> > > DOMMediaStream::OnTracksAvailableCallback and DOMMediaStream::TrackListener?
> > > Maybe we can combine them.
> > > 
> > > (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #10)
> > > > Comment on attachment 8746919 [details] [diff] [review]
> > > > Change HTMLMediaElement::StreamSizeListerner to inherit
> > > > MediaStreamTrackDirectListener.
> > > > 
> > > > Cancel review, I think I might not use OnTracksAvailableCallback.
Depends on: 1271566
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Carry r+
Attachment #8751099 - Flags: review?(rjesup)
Attachment #8751099 - Flags: review?(pehrsons)
Attachment #8751099 - Flags: review+
Attachment #8746919 - Attachment is obsolete: true
Comment on attachment 8746920 [details] [diff] [review]
Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Change to use MozReview in this bug.
Attachment #8746920 - Attachment is obsolete: true
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/1-2/
Attachment #8751099 - Flags: review?(rjesup)
Attachment #8751099 - Flags: review?(pehrsons)
Attachment #8751099 - Flags: review+
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/1-2/
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Carry r+, resubmit the patches due to bug 1271566 updated.
Attachment #8751099 - Flags: review?(rjesup)
Attachment #8751099 - Flags: review?(pehrsons)
Attachment #8751099 - Flags: review+
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

https://reviewboard.mozilla.org/r/51807/#review48757

This'll help MozReview understand when you publish next time.
Attachment #8751099 - Flags: review+
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review48767

::: dom/html/HTMLMediaElement.cpp:712
(Diff revision 2)
>  
> +/**
> + * This listener observes the first video frame to arrive with a non-empty size,
> + * and calls HTMLMediaElement::ReceivedMediaStreamInitialSize() with that size.
> + */
> +class HTMLMediaElement::StreamSizeListener : public MediaStreamTrackDirectListener {

I think I'd prefer to have this class defined above all the HTMLMediaElement:: methods.

::: dom/html/HTMLMediaElement.cpp:3413
(Diff revision 2)
> +void HTMLMediaElement::RemoveStreamSizeListener() {
> +  bool found;
> +  VideoTrack* videoTrack = mVideoTrackList->
> +      IndexedGetter(mVideoTrackList->SelectedIndex(), found);
> +  MOZ_ASSERT(videoTrack);
> +  VideoStreamTrack* videoStreamTrack = videoTrack->GetVideoStreamTrack();
> +  MOZ_ASSERT(videoStreamTrack);
> +  videoStreamTrack->RemoveDirectListener(mMediaStreamSizeListener);
> +}

I think it makes more sense to inline this in a method we call whenever the SelectedIndex changes.

This includes changing the SelectedIndex when the src was reset and we're cleaning up.

Mainly because this has a number of holes in it. Mainly you cannot guarantee or check in this method that the current SelectedIndex() refers to the track to which the listener was added.

::: dom/html/HTMLMediaElement.cpp:3525
(Diff revision 2)
> +    // New MediaStreamTrack added, we should re-construct the MediaTracks in
> +    // the HTMLMediaElement.
> +    ConstructMediaTracks();

You say a MediaStreamTrack was added, but you only do this for VideoStreamTracks.

Also I'm not sure reconstructing the MediaTracks makes sense. Just add the new track instead. I think the track list is hooked up directly to the TrackListener for doing that right now, but you could move the logic to the HTMLMediaElement if you have to.

::: dom/media/MediaStreamGraph.h:334
(Diff revision 2)
> -   *    This is the failure when you install the listener to a non-audio track.
> +   *    This is the failure when you install the listener to a
> +   *    non-(audio or video) track.

This is a bigger change that will for instance switch MediaPipeline to use direct listeners.

I'm not sure we want that, but jesup can make the call.

If we do want that, then remove TRACK_TYPE_NOT_SUPPORTED completely since we only have audio and video track types.

::: dom/media/MediaTrackList.h:65
(Diff revision 2)
>    static already_AddRefed<VideoTrack>
>    CreateVideoTrack(const nsAString& aId,
>                     const nsAString& aKind,
>                     const nsAString& aLabel,
> -                   const nsAString& aLanguage);
> +                   const nsAString& aLanguage,
> +                   VideoStreamTrack* aVideoTrack = nullptr);

Add a comment somewhere about what a null VideoStreamTrack means (src was not a MediaStream).

::: dom/media/VideoTrack.h:41
(Diff revision 2)
>    // default. If multiple video tracks are selected by its media resource at
>    // fetching phase, then the first enabled video track is set selected.
>    // aFlags contains FIRE_NO_EVENTS because no events are fired in such cases.
>    void SetEnabledInternal(bool aEnabled, int aFlags) override;
>  
> +  void SetVideoStreamTrack(VideoStreamTrack* aStreamTarck);

Shouldn't the constructor be enough for setting the track?

::: dom/media/VideoTrack.h:58
(Diff revision 2)
>    // must be unselected.
>    void SetSelected(bool aSelected);
>  
>  private:
>    bool mSelected;
> +  RefPtr<VideoStreamTrack> mVideoStreamTrack;

I think this should participate in CC traversal.

Perhaps also add a comment as to what it is doing here.
Attachment #8751098 - Flags: review?(pehrsons)
Hi, Ranedll,
Do you think should we relax the constraint of DirectMediaStreamTrackListener to video?
If so, should we remove TRACK_TYPE_NOT_SUPPORTED? We might want to support depth track in some day. But I am fine to remove TRACK_TYPE_NOT_SUPPORTED if you think the support of depth should not be considered.
Also if you think we can make DirectMediaStreamTrackListener to video, I think I might file a new bug for it and fix it.

> ::: dom/media/MediaStreamGraph.h:334
> (Diff revision 2)
> > -   *    This is the failure when you install the listener to a non-audio track.
> > +   *    This is the failure when you install the listener to a
> > +   *    non-(audio or video) track.
> 
> This is a bigger change that will for instance switch MediaPipeline to use
> direct listeners.
> 
> I'm not sure we want that, but jesup can make the call.
> 
> If we do want that, then remove TRACK_TYPE_NOT_SUPPORTED completely since we
> only have audio and video track types.
>
Flags: needinfo?(rjesup)
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/2-3/
Attachment #8751098 - Flags: review?(pehrsons)
Attachment #8751099 - Flags: review+ → review?(rjesup)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/2-3/
Attachment #8751099 - Flags: review?(rjesup) → review+
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

https://reviewboard.mozilla.org/r/51807/#review49463
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review49469

Basically r+ once pehrsons' other comments are dealt with

::: dom/media/MediaStreamGraph.h:333
(Diff revision 3)
>     * TRACK_TYPE_NOT_SUPPORTED
> -   *    This is the failure when you install the listener to a non-audio track.
> +   *    This is the failure when you install the listener to a
> +   *    non-(audio or video) track.

As Andreas says, this is a larger change, and means that video data will be fed through immediately to receivers (when possible) on the thread calling AppendToTrack.

This can be a good thing - waiting for NotifyQueuedTrackChanges/etc means waiting for the next iteration of the graph (perhaps 0-10ms typically).  And video code typically doesn't process it on the Notify trhead anyways, since we can't load the MSG thread, so doing the same on the caller's thread should be ok.  

This may slightly improve A/V sync, and cut MSG overhead a little (since DirectListeners can ignore the NotifyQueuedTrackChanges entirely.

I'm good with trying this; we can remove NOT_SUPPORTED if you like but if so, let's just add an assert to replace it, so if we try to add another type it will flag it.
Attachment #8751098 - Flags: review?(rjesup)
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review49698

::: dom/html/HTMLMediaElement.cpp:3415
(Diff revision 3)
> +void HTMLMediaElement::RemoveStreamSizeListener() {
> +  MOZ_ASSERT(mSelectedVideoStreamTrack);
> +  mSelectedVideoStreamTrack->RemoveDirectListener(mMediaStreamSizeListener);
> +  mSelectedVideoStreamTrack = nullptr;

Removing the StreamSizeListener shouldn't imply clearing mSelectedVideoStreamTrack.

The selected video track is still selected after the stream size listener has done its part.

And I'd still like this to be inlined (it's really just `mSelectedVideoStreamTrack->RemoveDirectListener()`)

::: dom/html/HTMLMediaElement.cpp:3522
(Diff revision 3)
> +    int32_t selectedIndex = VideoTracks()->SelectedIndex();
>      RefPtr<VideoTrack> videoTrack = CreateVideoTrack(t);
>      VideoTracks()->AddTrack(videoTrack);
> +    // New MediaStreamTrack added, set the new added video track as selected
> +    // video track when there is no selected track.
> +    if (selectedIndex == -1) {
> +      videoTrack->SetEnabledInternal(true, MediaTrack::FIRE_NO_EVENTS);
> +      mMediaStreamSizeListener = new StreamSizeListener(this);
> +      t->AddDirectListener(mMediaStreamSizeListener);
> +      mSelectedVideoStreamTrack = t;
> +    }
> +

Actually the spec says
> Similarly, a single VideoTrack object per VideoTrackList object can be selected, this is the video track's selection state. When a VideoTrack is created, its selection state must be set to false (not selected). The resource fetch algorithm can override this.

So we should only set the internal enabled state in ConstructMediaTracks().

I know you're doing it this way because we don't yet support MediaTracks and because of legacy behavior. Please then file a bug to fix this per the spec before preffing on MediaTracks, and comment on that bug number here.

::: dom/html/HTMLMediaElement.cpp:3527
(Diff revision 3)
> +    int32_t selectedIndex = VideoTracks()->SelectedIndex();
>      RefPtr<VideoTrack> videoTrack = CreateVideoTrack(t);
>      VideoTracks()->AddTrack(videoTrack);
> +    // New MediaStreamTrack added, set the new added video track as selected
> +    // video track when there is no selected track.
> +    if (selectedIndex == -1) {

Also assert that mSelectedVideoTrack is null when selectedIndex is -1.

::: dom/html/HTMLMediaElement.cpp:3552
(Diff revision 3)
> +    // If the removed media stream track is selected video track and there are
> +    // still video tracks, change the selected video track to the first
> +    // remaining track.

Likewise here, add a bug number that explains that we should fix this behavior up per the spec.

::: dom/html/HTMLMediaElement.cpp:3555
(Diff revision 3)
> +    if (aTrack == mSelectedVideoStreamTrack) {
> +      RemoveStreamSizeListener();
> +      mMediaStreamSizeListener->Forget();
> +      mMediaStreamSizeListener = nullptr;
> +      if (VideoTracks()->Length() > 0 && mSrcStream) {
> +        nsTArray<RefPtr<VideoStreamTrack>> tracks;
> +        mSrcStream->GetVideoTracks(tracks);
> +
> +        int firstEnabledVideo = -1;
> +        for (const RefPtr<VideoStreamTrack>& track : tracks) {
> +          if (track->Ended()) {
> +            continue;
> +          }
> +          if (track->Enabled()) {
> +            nsAutoString selectedId;
> +            track->GetId(selectedId);
> +            MediaTrack* videoTrack = VideoTracks()->GetTrackById(selectedId);
> +            MOZ_ASSERT(videoTrack);
> +            videoTrack->SetEnabledInternal(true, MediaTrack::FIRE_NO_EVENTS);
> +            mMediaStreamSizeListener = new StreamSizeListener(this);
> +            track->AddDirectListener(mMediaStreamSizeListener);
> +            mSelectedVideoStreamTrack = track;
> +            return;
> +          }
> +        }
> +      }
> +    }

Make this a bit easier to read by flipping some ifs to be guards instead.

You also don't need the protection for an empty VideoTracks() since you are looping through that list anyway.

::: dom/html/HTMLMediaElement.cpp:3557
(Diff revision 3)
> +      mMediaStreamSizeListener->Forget();
> +      mMediaStreamSizeListener = nullptr;

If you clean up mMediaStreamSizeListener at the end instead...

::: dom/html/HTMLMediaElement.cpp:3574
(Diff revision 3)
> +            mMediaStreamSizeListener = new StreamSizeListener(this);
> +            track->AddDirectListener(mMediaStreamSizeListener);

...you should be able to reuse the existing mMediaStreamSizeListener here.

::: dom/media/MediaStreamGraph.cpp:2865
(Diff revision 3)
>                                     listener.get()));
>      listener->NotifyDirectListenerInstalled(
>        MediaStreamTrackDirectListener::InstallationResult::TRACK_NOT_FOUND_AT_SOURCE);
>      return;
>    }
> -  if (!isAudio) {
> +  if (!(isAudio || isVideo)) {

I find !isAudio && !isVideo easier to read.

::: dom/media/MediaTrackList.h:62
(Diff revision 3)
> +  // For the case of src of HTMLMediaElement is MediaStream, leave the
> +  // aVideoTrack as default(nullptr).

You mean the opposite?
Attachment #8751098 - Flags: review?(pehrsons)
https://reviewboard.mozilla.org/r/51805/#review49469

> As Andreas says, this is a larger change, and means that video data will be fed through immediately to receivers (when possible) on the thread calling AppendToTrack.
> 
> This can be a good thing - waiting for NotifyQueuedTrackChanges/etc means waiting for the next iteration of the graph (perhaps 0-10ms typically).  And video code typically doesn't process it on the Notify trhead anyways, since we can't load the MSG thread, so doing the same on the caller's thread should be ok.  
> 
> This may slightly improve A/V sync, and cut MSG overhead a little (since DirectListeners can ignore the NotifyQueuedTrackChanges entirely.
> 
> I'm good with trying this; we can remove NOT_SUPPORTED if you like but if so, let's just add an assert to replace it, so if we try to add another type it will flag it.

I think those(video part) will be processed in stop-buffering bug. The MediaStreamVideoSink will be kind of DirectMediaStreamTrackListener since Andreas suggested me to leverge the existing adding direct track mechanism. Those codes basically work though still need to change the way of adding MediaStreamVideoSink after clone bug landed.

I agree with using assert. Thanks for the suggestion.
Depends on: 1273443
https://reviewboard.mozilla.org/r/51805/#review49698

> Actually the spec says
> > Similarly, a single VideoTrack object per VideoTrackList object can be selected, this is the video track's selection state. When a VideoTrack is created, its selection state must be set to false (not selected). The resource fetch algorithm can override this.
> 
> So we should only set the internal enabled state in ConstructMediaTracks().
> 
> I know you're doing it this way because we don't yet support MediaTracks and because of legacy behavior. Please then file a bug to fix this per the spec before preffing on MediaTracks, and comment on that bug number here.

Filed Bug 1273443 for spec issue. Although bug 744896 is landed, but still pref off media.track.enabled, see [1].

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1119299#c3
https://reviewboard.mozilla.org/r/51805/#review49698

> Make this a bit easier to read by flipping some ifs to be guards instead.
> 
> You also don't need the protection for an empty VideoTracks() since you are looping through that list anyway.

I think you misunderstanding the protection of VideoTracks(). The looping is for an array of VideoStreamTrack which is different from VideoTracks(). Those are two different object. HTMLMediaElement contains mVideoTrackList, an object containing an array of VideoTrack. The looping is for an array of VideoStreamTrack. This array of VideoStreamTrack is owned by HTMLMediaElement::mSrcStream as mTracks in DOMMediaStream. And VideoStreamTrack is also different form VideoTrack.
Per talk with Andreas on IRC, it seems he know the difference of VideoTracks and VideoStreamTrack. And he assume both should be synced in this case. So I will try to address his comment.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #32)
> https://reviewboard.mozilla.org/r/51805/#review49698
> 
> > Make this a bit easier to read by flipping some ifs to be guards instead.
> > 
> > You also don't need the protection for an empty VideoTracks() since you are looping through that list anyway.
> 
> I think you misunderstanding the protection of VideoTracks(). The looping is
> for an array of VideoStreamTrack which is different from VideoTracks().
> Those are two different object. HTMLMediaElement contains mVideoTrackList,
> an object containing an array of VideoTrack. The looping is for an array of
> VideoStreamTrack. This array of VideoStreamTrack is owned by
> HTMLMediaElement::mSrcStream as mTracks in DOMMediaStream. And
> VideoStreamTrack is also different form VideoTrack.
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/3-4/
Attachment #8751098 - Flags: review?(rjesup)
Attachment #8751098 - Flags: review?(pehrsons)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/3-4/
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review50554

These are all pretty much nits

::: dom/html/HTMLMediaElement.cpp:3565
(Diff revisions 3 - 4)
> -          if (track->Ended()) {
> -            continue;
> +        if (track->Ended()) { continue; }
> +
> -          }

Change this back to multiple lines

::: dom/html/HTMLMediaElement.cpp:4501
(Diff revisions 3 - 4)
> -  RemoveStreamSizeListener();
> +  mSelectedVideoStreamTrack->RemoveDirectListener(mMediaStreamSizeListener);
> +//  mSelectedVideoStreamTrack = nullptr;

why the commented-out line?

::: dom/media/MediaStreamGraph.cpp:2865
(Diff revisions 3 - 4)
> -  if (!(isAudio || isVideo)) {
> +  if (!isAudio && !isVideo) {
>      STREAM_LOG(LogLevel::Warning, ("Source track for direct track listener %p is not audio",
>                                     listener.get()));

"is unknown" not "is not audio"

::: dom/media/VideoTrackList.cpp:32
(Diff revision 3)
>    if (!(found && videoTrack && aTrack == videoTrack)) {
>      mSelectedIndex = -1;
>      return;
>    }
>  
>    for (size_t ix = 0; ix < mTracks.Length(); ix ++) {
> -    if (mTracks[ix] == aTrack) {
> +    if (mTracks[ix] == videoTrack) {
>        mSelectedIndex = ix;
>        return;

is this change needed?  aTrack == videoTrack, so it's not for correctness.  If it's being doen to be simpler to understand or read better, then that's fine.
Attachment #8751098 - Flags: review?(rjesup) → review+
https://reviewboard.mozilla.org/r/51805/#review50554

> why the commented-out line?

Andreas suggest that "Removing the StreamSizeListener shouldn't imply clearing mSelectedVideoStreamTrack." And it seems when |UpdateInitialMediaSize| called, we don't change the array of VideoTracks(). So that means we can keep the mSelectedVideoStreamTrack the same. And I forget to delete this line when I trying to inline the function |RemoveStreamSizeListener|.
https://reviewboard.mozilla.org/r/51805/#review50554

> is this change needed?  aTrack == videoTrack, so it's not for correctness.  If it's being doen to be simpler to understand or read better, then that's fine.

It is for correctness. I made a mistake in last patch. Basically this deal with the case when removing the non currently-selected track. We need to find the new position of currently-selected track. Maybe I should change the name of videoTrack to selectedVideoTrack for better readness.
Depends on: 1274221
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review50636

r+ with nits fixed.

::: dom/html/HTMLMediaElement.cpp:782
(Diff revision 4)
>    bool fireTimeUpdate = false;
>  
> +  // We need to remove StreamSizeListener before VideoTracks get emptied.
> +  if (mMediaStreamSizeListener) {
> +    mSelectedVideoStreamTrack->RemoveDirectListener(mMediaStreamSizeListener);
> +    mSelectedVideoStreamTrack = nullptr;

This line could be outside the if.

::: dom/html/HTMLMediaElement.cpp:3424
(Diff revision 4)
>  
>    UpdateSrcMediaStreamPlaying(REMOVING_SRC_STREAM);
>  
>    if (mMediaStreamSizeListener) {
> -    RefPtr<MediaStream> stream = GetSrcMediaStream();
> -    if (stream) {
> +    mSelectedVideoStreamTrack->RemoveDirectListener(mMediaStreamSizeListener);
> +    mSelectedVideoStreamTrack = nullptr;

This could be outside the if as well.

::: dom/html/HTMLMediaElement.cpp:3564
(Diff revision 4)
> +      for (const RefPtr<VideoStreamTrack>& track : tracks) {
> +        if (track->Ended()) { continue; }
> +
> +        if (track->Enabled()) {

Make it a guard here: `if (!track->Enabled()) { continue; }`

::: dom/html/HTMLMediaElement.cpp:3568
(Diff revision 4)
> +          nsAutoString selectedId;
> +          track->GetId(selectedId);

I'm confused by this `selectedId` since it comes from `track` that doesn't have a notion of "selected" yet. Can we call it `trackId` instead?

::: dom/media/VideoTrack.h:44
(Diff revision 4)
>    // default. If multiple video tracks are selected by its media resource at
>    // fetching phase, then the first enabled video track is set selected.
>    // aFlags contains FIRE_NO_EVENTS because no events are fired in such cases.
>    void SetEnabledInternal(bool aEnabled, int aFlags) override;
>  
> +  VideoStreamTrack* GetVideoStreamTrack() { return mVideoStreamTrack; }

This deserves a comment (and mention when it may be null).
Attachment #8751098 - Flags: review?(pehrsons) → review+
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/4-5/
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/4-5/
Depends on: 1275201
No longer depends on: 1275201
Andreas, 
Please help me to land this bug after bug 1208373 and bug 1274221 landed. :)
Flags: needinfo?(pehrsons)
Flags: needinfo?(pehrson)
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c235b056514
Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener. r=jesup, r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89b8cc657b2
Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively. r=jesup,pehrsons
Backed out for frequently crashing in mda's test_streams_element_capture_createObjectURL.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/823b470e5aacc0c2426d2563197e0ae90a36dd53
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf789b359fcd281a8532469a006be63e06705932

Push showing these failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1bd6da31483db28d8fd65a0ef69d8dfe42cb0f0f
Example: https://treeherder.mozilla.org/logviewer.html#?job_id=30087711&repo=mozilla-inbound

11:21:33     INFO -  1565 INFO TEST-START | dom/media/test/test_streams_element_capture_createObjectURL.html
11:21:34     INFO -  TEST-INFO | Main app process: exit 1
11:21:34     INFO -  1566 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | A valid string reason is expected
11:21:34     INFO -  1567 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | Reason cannot be empty
11:21:34     INFO -  1568 INFO Started Mon Jun 13 2016 11:21:33 GMT-0700 (Pacific Standard Time) (1465842093.425s)
11:21:34     INFO -  1569 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | [started 320x240.ogv-0 t=0.014] Length of array should match number of running tests
11:21:34     INFO -  1570 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | 320x240.ogv stream initial currentTime
11:21:34     INFO -  1571 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | 320x240.ogv stream final currentTime
11:21:34     INFO -  1572 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | Got 0.26702947845804986, expected at least 0.266; 320x240.ogv current time at end
11:21:34     INFO -  1573 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | 320x240.ogv checking readyState
11:21:34     INFO -  1574 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | 320x240.ogv checking playback has ended
11:21:34     INFO -  1575 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | Check video frame pixel has been drawn
11:21:34     INFO -  1576 INFO [finished 320x240.ogv-0] remaining=
11:21:34     INFO -  1577 INFO TEST-PASS | dom/media/test/test_streams_element_capture_createObjectURL.html | [finished 320x240.ogv-0 t=0.285] Length of array should match number of running tests
11:21:34  WARNING -  TEST-UNEXPECTED-FAIL | dom/media/test/test_streams_element_capture_createObjectURL.html | application terminated with exit code 1
[...]
11:21:48  WARNING -  PROCESS-CRASH | dom/media/test/test_streams_element_capture_createObjectURL.html | application crashed [@ mozilla::dom::MediaStreamTrack::RemoveDirectListener(mozilla::DirectMediaStreamTrackListener *)]
11:21:48     INFO -  Crash dump filename: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpdtxo3d.mozrunner\minidumps\62b4bd15-6d0a-4858-956a-008d4e7a6489.dmp
11:21:48     INFO -  Operating system: Windows NT
11:21:48     INFO -                    5.1.2600 Service Pack 3
11:21:48     INFO -  CPU: x86
11:21:48     INFO -       GenuineIntel family 6 model 30 stepping 5
11:21:48     INFO -       8 CPUs
11:21:48     INFO -  Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
11:21:48     INFO -  Crash address: 0x38
11:21:48     INFO -  Process uptime: 730 seconds
11:21:48     INFO -  Thread 0 (crashed)
11:21:48     INFO -   0  xul.dll!mozilla::dom::MediaStreamTrack::RemoveDirectListener(mozilla::DirectMediaStreamTrackListener *) [MediaStreamTrack.cpp:1bd6da31483d : 428 + 0x9]
11:21:48     INFO -      eip = 0x03fda278   esp = 0x0012d898   ebp = 0x0012d8a4   ebx = 0x00000000
11:21:48     INFO -      esi = 0x1188ad00   edi = 0x1d3c4000   eax = 0x00000000   ecx = 0x16b38660
11:21:48     INFO -      edx = 0x1d38b790   efl = 0x00010297
11:21:48     INFO -      Found by: given as instruction pointer in context
11:21:48     INFO -   1  xul.dll!mozilla::dom::HTMLMediaElement::EndSrcMediaStreamPlayback() [HTMLMediaElement.cpp:1bd6da31483d : 3427 + 0xc]
11:21:48     INFO -      eip = 0x03f31133   esp = 0x0012d8ac   ebp = 0x0012d8c8
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   2  xul.dll!mozilla::dom::HTMLMediaElement::cycleCollection::Unlink(void *) [HTMLMediaElement.cpp:1bd6da31483d : 544 + 0x7]
11:21:48     INFO -      eip = 0x03f49908   esp = 0x0012d8c0   ebp = 0x0012d8c8
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   3  xul.dll!nsCycleCollector::CollectWhite() [nsCycleCollector.cpp:1bd6da31483d : 3343 + 0xb]
11:21:48     INFO -      eip = 0x0325ea4a   esp = 0x0012d8d0   ebp = 0x0012d91c
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   4  xul.dll!nsCycleCollector::Collect(ccType,js::SliceBudget &,nsICycleCollectorListener *,bool) [nsCycleCollector.cpp:1bd6da31483d : 3689 + 0x7]
11:21:48     INFO -      eip = 0x0325e5a0   esp = 0x0012d924   ebp = 0x0012d950
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   5  xul.dll!nsCycleCollector_collect(nsICycleCollectorListener *) [nsCycleCollector.cpp:1bd6da31483d : 4160 + 0x12]
11:21:48     INFO -      eip = 0x03264104   esp = 0x0012d958   ebp = 0x0012d990
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   6  xul.dll!nsJSContext::CycleCollectNow(nsICycleCollectorListener *,int) [nsJSEnvironment.cpp:1bd6da31483d : 1435 + 0x8]
11:21:48     INFO -      eip = 0x03a5db47   esp = 0x0012d998   ebp = 0x0012d9a4
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   7  xul.dll!nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener *,int) [nsDOMWindowUtils.cpp:1bd6da31483d : 1245 + 0xb]
11:21:48     INFO -      eip = 0x039bc65a   esp = 0x0012d9ac   ebp = 0x0012d9b4
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   8  xul.dll!NS_InvokeByIndex + 0x27
11:21:48     INFO -      eip = 0x0329def7   esp = 0x0012d9bc   ebp = 0x0012d9d0
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -   9  xul.dll!XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode) [XPCWrappedNative.cpp:1bd6da31483d : 1367 + 0xaa7]
11:21:48     INFO -      eip = 0x03711b05   esp = 0x0012d9d8   ebp = 0x0012db68
11:21:48     INFO -      Found by: previous frame's frame pointer
11:21:48     INFO -  10  xul.dll!js::TypeNewScript::maybeAnalyze(JSContext *,js::ObjectGroup *,bool *,bool) [TypeInference.cpp:1bd6da31483d : 3654 + 0x35]
11:21:48     INFO -      eip = 0x04d3002a   esp = 0x0012da08   ebp = 0x0012db68
11:21:48     INFO -      Found by: stack scanning
11:21:48     INFO -  11  xul.dll!NewArrayTryReuseGroup<4294967295> [jsarray.cpp:1bd6da31483d : 3622 + 0x1b]
11:21:48     INFO -      eip = 0x04d714e0   esp = 0x0012da28   ebp = 0x0012db68
11:21:48     INFO -      Found by: stack scanning
11:21:48     INFO -  12  xul.dll!NewArrayTryReuseGroup<4294967295> [jsarray.cpp:1bd6da31483d : 3623 + 0xa]
11:21:48     INFO -      eip = 0x04d714fd   esp = 0x0012da50   ebp = 0x0012db68
11:21:48     INFO -      Found by: stack scanning
11:21:48     INFO -  13  xul.dll!JS::Zone::getUniqueId(js::gc::Cell *,unsigned __int64 *) [Zone.h:1bd6da31483d : 417 + 0x25]
11:21:48     INFO -      eip = 0x04c53583   esp = 0x0012dacc   ebp = 0x0012db68
11:21:48     INFO -      Found by: stack scanning
11:21:48     INFO -  14  xul.dll!XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) [XPCWrappedNativeJSOps.cpp:1bd6da31483d : 1128 + 0xa]
11:21:48     INFO -      eip = 0x0371f30d   esp = 0x0012db70   ebp = 0x0012dc08
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  15  xul.dll!js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [Interpreter.cpp:1bd6da31483d : 452 + 0x5d]
11:21:48     INFO -      eip = 0x04cb56d3   esp = 0x0012dc10   ebp = 0x0012dc58
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  16  xul.dll!js::FastCallGuard::call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::MutableHandle<JS::Value>) [Interpreter-inl.h:1bd6da31483d : 850 + 0x9]
11:21:48     INFO -      eip = 0x04d5cbf9   esp = 0x0012dc60   ebp = 0x0012dc78
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  17  xul.dll!Reflect_apply [Reflect.cpp:1bd6da31483d : 79 + 0x26]
11:21:48     INFO -      eip = 0x04d3fceb   esp = 0x0012dc80   ebp = 0x0012dd38
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  18  xul.dll!js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [Interpreter.cpp:1bd6da31483d : 452 + 0x5d]
11:21:48     INFO -      eip = 0x04cb56d3   esp = 0x0012dd40   ebp = 0x0012dd88
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  19  xul.dll!InternalCall [Interpreter.cpp:1bd6da31483d : 497 + 0xb]
11:21:48     INFO -      eip = 0x04cb5534   esp = 0x0012dd90   ebp = 0x0012dda8
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  20  xul.dll!Interpret [Interpreter.cpp:1bd6da31483d : 2873 + 0x10]
11:21:48     INFO -      eip = 0x04cba567   esp = 0x0012ddb0   ebp = 0x0012e700
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  21  xul.dll!js::RunScript(JSContext *,js::RunState &) [Interpreter.cpp:1bd6da31483d : 398 + 0xd]
11:21:48     INFO -      eip = 0x04cc2020   esp = 0x0012e708   ebp = 0x0012e7a4
11:21:48     INFO -      Found by: previous frame's frame pointer
11:21:48     INFO -  22  xul.dll!js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [Interpreter.cpp:1bd6da31483d : 470 + 0xa]
11:21:48     INFO -      eip = 0x04cb575f   esp = 0x0012e7ac   ebp = 0x0012e7f0
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  23  xul.dll!InternalCall [Interpreter.cpp:1bd6da31483d : 497 + 0xb]
11:21:48     INFO -      eip = 0x04cb5534   esp = 0x0012e7f8   ebp = 0x0012e810
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  24  xul.dll!js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [Interpreter.cpp:1bd6da31483d : 516 + 0x5]
11:21:48     INFO -      eip = 0x04cb01c0   esp = 0x0012e818   ebp = 0x0012e824
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  25  xul.dll!js::ScriptedProxyHandler::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [ScriptedProxyHandler.cpp:1bd6da31483d : 1160 + 0x29]
11:21:48     INFO -      eip = 0x04ccf858   esp = 0x0012e82c   ebp = 0x0012e968
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  26  xul.dll!js::Proxy::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) [Proxy.cpp:1bd6da31483d : 401 + 0xe]
11:21:48     INFO -      eip = 0x04ccf578   esp = 0x0012e970   ebp = 0x0012e994
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  27  xul.dll!js::proxy_Call(JSContext *,unsigned int,JS::Value *) [Proxy.cpp:1bd6da31483d : 689 + 0x12]
11:21:48     INFO -      eip = 0x04cd5a5a   esp = 0x0012e99c   ebp = 0x0012e9c0
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  28  xul.dll!js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [Interpreter.cpp:1bd6da31483d : 440 + 0x4c]
11:21:48     INFO -      eip = 0x04cb562d   esp = 0x0012e9c8   ebp = 0x0012ea10
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  29  xul.dll!InternalCall [Interpreter.cpp:1bd6da31483d : 497 + 0xb]
11:21:48     INFO -      eip = 0x04cb5534   esp = 0x0012ea18   ebp = 0x0012ea30
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  30  xul.dll!Interpret [Interpreter.cpp:1bd6da31483d : 2873 + 0x10]
11:21:48     INFO -      eip = 0x04cba567   esp = 0x0012ea38   ebp = 0x0012f388
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  31  mozglue.dll!arena_run_split [jemalloc.c:1bd6da31483d : 3556 + 0xd]
11:21:48     INFO -      eip = 0x10004f13   esp = 0x0012ea70   ebp = 0x0012f388
11:21:48     INFO -      Found by: stack scanning
11:21:48     INFO -  32  xul.dll!js::RunScript(JSContext *,js::RunState &) [Interpreter.cpp:1bd6da31483d : 398 + 0xd]
11:21:48     INFO -      eip = 0x04cc2020   esp = 0x0012f390   ebp = 0x0012f428
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  33  xul.dll!js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [Interpreter.cpp:1bd6da31483d : 470 + 0xa]
11:21:48     INFO -      eip = 0x04cb575f   esp = 0x0012f430   ebp = 0x0012f474
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  34  xul.dll!InternalCall [Interpreter.cpp:1bd6da31483d : 497 + 0xb]
11:21:48     INFO -      eip = 0x04cb5534   esp = 0x0012f47c   ebp = 0x0012f494
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  35  xul.dll!js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [Interpreter.cpp:1bd6da31483d : 516 + 0x5]
11:21:48     INFO -      eip = 0x04cb01c0   esp = 0x0012f49c   ebp = 0x0012f4a8
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  36  xul.dll!JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) [jsapi.cpp:1bd6da31483d : 2829 + 0x18]
11:21:48     INFO -      eip = 0x04c2ab1f   esp = 0x0012f4b0   ebp = 0x0012f558
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  37  xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [XPCWrappedJSClass.cpp:1bd6da31483d : 1213 + 0x45]
11:21:48     INFO -      eip = 0x0371285e   esp = 0x0012f560   ebp = 0x0012f7bc
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  38  xul.dll!nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) [XPCWrappedJS.cpp:1bd6da31483d : 602 + 0x12]
11:21:48     INFO -      eip = 0x03711fd7   esp = 0x0012f7c4   ebp = 0x0012f7e0
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  39  xul.dll!PrepareAndDispatch [xptcstubs.cpp:1bd6da31483d : 85 + 0x1b]
11:21:48     INFO -      eip = 0x0329de72   esp = 0x0012f7e8   ebp = 0x0012f898
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  40  xul.dll!SharedStub [xptcstubs.cpp:1bd6da31483d : 112 + 0x5]
11:21:48     INFO -      eip = 0x0329d3a5   esp = 0x0012f8a0   ebp = 0x0012f8b4
11:21:48     INFO -      Found by: call frame info
11:21:48     INFO -  41  xul.dll!PreciseGCRunnable::Run() [XPCComponents.cpp:1bd6da31483d : 2647 + 0x6]
11:21:48     INFO -      eip = 0x036f2240   esp = 0x0012f8bc   ebp = 0x0012f8b4
11:21:48     INFO -      Found by: call frame info
Flags: needinfo?(ctai)
Looks like it is caused by the sequence of unlink is random. If the unlink function of MediaStreamTrack is called earlier than the unlink function of HTMLMediaElement, the crash happens. I will find a better way to clean the listener.
Flags: needinfo?(ctai)
Flags: needinfo?(rjesup)
This can reduce the include header dependency. MediaStreamVideoSink will inherit from DirectMediaStreamTrackListener. But we can't use forward declaration on MediaStreamListener because the usage of nsTArray<RefPtr<MediaStreamVideoSink>>.

Review commit: https://reviewboard.mozilla.org/r/59438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59438/
Attachment #8751099 - Attachment description: MozReview Request: Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively. r=jesup,pehrsons → Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.
Attachment #8751098 - Attachment description: MozReview Request: Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener. r?jesup,pehrsons → Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.
Attachment #8763115 - Flags: review?(rjesup)
Attachment #8763115 - Flags: review?(pehrson)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/5-6/
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/5-6/
Attachment #8751098 - Flags: review?(rjesup)
Attachment #8751098 - Flags: review?(pehrson)
Attachment #8751098 - Flags: review+
Attachment #8751099 - Flags: review?(rjesup)
Attachment #8751099 - Flags: review?(pehrson)
Attachment #8751099 - Flags: review+
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #48)
> Comment on attachment 8751098 [details]
> Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit
> MediaStreamTrackDirectListener.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/51805/diff/5-6/

The try looks good to me.
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7ff64a19957
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

https://reviewboard.mozilla.org/r/59438/#review56610

> But we can't use forward declaration on MediaStreamListener because the usage of nsTArray<RefPtr<MediaStreamVideoSink>>.

We can if we move the constructor, destructor and any accessors of that array to the cpp file.

::: dom/media/MediaStreamGraph.h:9
(Diff revision 1)
> +#include "MediaStreamListener.h"
> +

Double include.

::: dom/media/MediaStreamGraph.h:20
(Diff revision 1)
>  
> -#include "AudioSegment.h"
>  #include "AudioStream.h"
>  #include "nsTArray.h"
>  #include "nsIRunnable.h"
> -#include "StreamTracks.h"
> +#include "MediaStreamListener.h"

While you're at it, move the include to the cpp and forward declare.

::: dom/media/MediaStreamListener.h:138
(Diff revision 1)
> +class AudioDataListenerInterface {
> +protected:
> +  // Protected destructor, to discourage deletion outside of Release():
> +  virtual ~AudioDataListenerInterface() {}
> +
> +public:
> +  /* These are for cubeb audio input & output streams: */
> +  /**
> +   * Output data to speakers, for use as the "far-end" data for echo
> +   * cancellation.  This is not guaranteed to be in any particular size
> +   * chunks.
> +   */
> +  virtual void NotifyOutputData(MediaStreamGraph* aGraph,
> +                                AudioDataValue* aBuffer, size_t aFrames,
> +                                TrackRate aRate, uint32_t aChannels) = 0;
> +  /**
> +   * Input data from a microphone (or other audio source.  This is not
> +   * guaranteed to be in any particular size chunks.
> +   */
> +  virtual void NotifyInputData(MediaStreamGraph* aGraph,
> +                               const AudioDataValue* aBuffer, size_t aFrames,
> +                               TrackRate aRate, uint32_t aChannels) = 0;
> +
> +  /**
> +   * Called when the underlying audio device has changed.
> +   */
> +  virtual void DeviceChanged() = 0;
> +};
> +
> +class AudioDataListener : public AudioDataListenerInterface {
> +protected:
> +  // Protected destructor, to discourage deletion outside of Release():
> +  virtual ~AudioDataListener() {}
> +
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AudioDataListener)
> +};

I think these should remain in MediaStreamGraph for now. They don't have anything to do with MediaStreams.
Attachment #8763115 - Flags: review?(pehrson)
Attachment #8763115 - Flags: review?(rjesup)
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

https://reviewboard.mozilla.org/r/59438/#review56656

See Andreas's comments; beyond what he says it looks good to me.  Please send any re-review requests to Andreas as I'm on PTO, thanks
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review56880

Cancelling this until I can see a diff.
Attachment #8751098 - Flags: review?(pehrson)
Attachment #8751099 - Flags: review?(pehrson)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

https://reviewboard.mozilla.org/r/51807/#review56882

Cancelling this until I can see a diff.
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/6-7/
Attachment #8751099 - Flags: review?(rjesup) → review?(pehrson)
Attachment #8763115 - Flags: review?(pehrson)
Attachment #8751098 - Flags: review?(rjesup) → review?(pehrson)
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59438/diff/1-2/
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/6-7/
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #56)
> Comment on attachment 8751098 [details]
> Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit
> MediaStreamTrackDirectListener.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/51805/diff/6-7/

Don't land this. I got a rebased one.
Attachment #8751098 - Flags: review?(pehrson) → review+
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

https://reviewboard.mozilla.org/r/51805/#review57620

r+ with some nits left to fix.

Also the diff is polluted by the MediaStreamListener.h changes that came in before this patch in the latest rev. I've just checked per my previous comments.

::: dom/html/HTMLMediaElement.cpp:3566
(Diff revisions 4 - 7)
> -        if (track->Ended()) { continue; }
> +        if (track->Ended() || !track->Enabled()) {
> +          continue;
> +        }

For diffing and readability, make the guards separate.

::: dom/html/HTMLMediaElement.cpp:3570
(Diff revisions 4 - 7)
>        for (const RefPtr<VideoStreamTrack>& track : tracks) {
> -        if (track->Ended()) { continue; }
> +        if (track->Ended() || !track->Enabled()) {
> +          continue;
> +        }
>  
>          if (track->Enabled()) {

Now that you have a guard you can remove this if.
Attachment #8751099 - Flags: review?(pehrson)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

https://reviewboard.mozilla.org/r/51807/#review57622

::: dom/html/HTMLMediaElement.cpp:712
(Diff revision 2)
>  
>  /**
>   * This listener observes the first video frame to arrive with a non-empty size,
>   * and calls HTMLMediaElement::ReceivedMediaStreamInitialSize() with that size.
>   */
> -class HTMLMediaElement::StreamSizeListener : public MediaStreamTrackDirectListener {
> +class HTMLMediaElement::StreamSizeListener : public DirectMediaStreamTrackListener {

In the rev 2-7 diff a couple of things seem reverted. Is that as intended?
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

https://reviewboard.mozilla.org/r/59438/#review57624

I'd like to see a cleanup in includes in header files, and getting rid of the EnumValue/ToEnum stuff. Apart from that, a couple of nits and we're good.

::: dom/media/MediaStreamGraph.h:16
(Diff revisions 1 - 2)
> +#include "AudioSegment.h"
> +#include "VideoSegment.h"
> +#include "StreamTracks.h"

It would be splendid if we could put these in the cpp as well. It shouldn't be harder than moving definitions that depend on them to the cpp.

::: dom/media/MediaStreamListener.h:10
(Diff revisions 1 - 2)
>  #include "AudioSegment.h"
>  #include "VideoSegment.h"
>  #include "StreamTracks.h"

Could you create MediaStreamListener.cpp and move these includes and all needed definitions there?

It'll help with our crazy recompile times when touching one of those low-level modules, and if we don't do it now we'll never get to it.

::: dom/media/MediaStreamListener.h:19
(Diff revisions 1 - 2)
> +// Utility functions for transform between enum class and the underlying value.
> +template <typename E>
> +constexpr typename std::underlying_type<E>::type EnumValue(E val)
> +{
> +    return static_cast<typename std::underlying_type<E>::type>(val);
> +}
> +
> +template< typename E , typename T>
> +constexpr inline typename std::enable_if<std::is_enum<E>::value &&
> +                                         std::is_integral<T>::value, E
> +                                         >::type
> +ToEnum(T value) noexcept
> +{
> +   return static_cast<E>(value);
> +}

Why do we need these?

And if we really do, they seem generic enough to go into mfbt/ or some such.

::: dom/media/MediaStreamListener.h:43
(Diff revisions 1 - 2)
> +enum class TrackEventCommand {
> +  TRACK_EVENT_NONE = 0x00,

I guess making this an enum class is what is causing all the headache. I'm fine with leaving it as a regular enum inside MediaStreamListener (i.e., same as before) since it's a flag set.

Sure you lose some type safety but we're not regressing anything either.

::: dom/camera/CameraPreviewMediaStream.cpp:93
(Diff revision 2)
>  {
>    MutexAutoLock lock(mMutex);
>  
>    RefPtr<MediaStreamListener> listener(aListener);
>    mListeners.RemoveElement(aListener);
> -  listener->NotifyEvent(mFakeMediaStreamGraph, MediaStreamListener::EVENT_REMOVED);
> +  listener->NotifyEvent(mFakeMediaStreamGraph, MediaStreamGraphEvent::EVENT_REMOVED);

Why change scope of these events? I think the point was to keep MediaStreamListener events within the MediaStreamListener scope.

::: dom/camera/CameraPreviewMediaStream.cpp:107
(Diff revision 2)
>        mTrackCreated = true;
>        VideoSegment tmpSegment;
>        for (uint32_t j = 0; j < mListeners.Length(); ++j) {
>          MediaStreamListener* l = mListeners[j];
>          l->NotifyQueuedTrackChanges(mFakeMediaStreamGraph, TRACK_VIDEO, 0,
> -                                    MediaStreamListener::TRACK_EVENT_CREATED,
> +                                    TrackEventCommand::TRACK_EVENT_CREATED,

Ditto.

::: dom/media/MediaManager.h:35
(Diff revision 2)
>  #include "mozilla/media/MediaChild.h"
>  #include "mozilla/media/MediaParent.h"
>  #include "mozilla/Logging.h"
>  #include "mozilla/UniquePtr.h"
>  #include "DOMMediaStream.h"
> +#include "MediaStreamListener.h"

Move to cpp.

::: dom/media/MediaManager.h:274
(Diff revision 2)
>    void
>    NotifyEvent(MediaStreamGraph* aGraph,
> -              MediaStreamListener::MediaStreamGraphEvent aEvent) override
> +              MediaStreamGraphEvent aEvent) override
>    {

The definition of this will inevitably have to move to the cpp file as well.

::: dom/media/MediaManager.h:286
(Diff revision 2)
>            NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyFinished));
>          break;
> -      case EVENT_REMOVED:
> +      case MediaStreamGraphEvent::EVENT_REMOVED:
>          NS_DispatchToMainThread(
>            NewRunnableMethod(this, &GetUserMediaCallbackMediaStreamListener::NotifyRemoved));
> -        break;
> +      break;

This one seems off now.

::: dom/media/encoder/MediaEncoder.h:13
(Diff revision 2)
>  
>  #include "mozilla/DebugOnly.h"
>  #include "TrackEncoder.h"
>  #include "ContainerWriter.h"
>  #include "MediaStreamGraph.h"
> +#include "MediaStreamListener.h"

-> cpp

::: dom/media/imagecapture/CaptureTask.h:11
(Diff revision 2)
>  
>  #ifndef CAPTURETASK_H
>  #define CAPTURETASK_H
>  
>  #include "MediaStreamGraph.h"
> +#include "MediaStreamListener.h"

-> cpp

::: dom/media/webspeech/recognition/SpeechStreamListener.h:11
(Diff revision 2)
>  
>  #ifndef mozilla_dom_SpeechStreamListener_h
>  #define mozilla_dom_SpeechStreamListener_h
>  
>  #include "MediaStreamGraph.h"
> +#include "MediaStreamListener.h"

-> cpp
Attachment #8763115 - Flags: review?(pehrson)
https://reviewboard.mozilla.org/r/51807/#review57622

> In the rev 2-7 diff a couple of things seem reverted. Is that as intended?

I changed the order of the patch sequence. I moved the patch "Rename MediaStreamDirectListener and MediaStreamTrackDirectListener...." in front of "Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner ....". So that cause the diff reverted. But I think the diff 0-7 is fine.
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Please review again. Thanks.
Attachment #8751099 - Flags: review?(pehrson)
https://reviewboard.mozilla.org/r/51807/#review57622

> I changed the order of the patch sequence. I moved the patch "Rename MediaStreamDirectListener and MediaStreamTrackDirectListener...." in front of "Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner ....". So that cause the diff reverted. But I think the diff 0-7 is fine.

I think I see how it happened now. Sorry for the delay.
Attachment #8751099 - Flags: review?(pehrson) → review+
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

https://reviewboard.mozilla.org/r/51807/#review57820
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #60)
> > +// Utility functions for transform between enum class and the underlying value.

I didn't look too closely at this but mfbt has EnumSet, which might be what you want.
https://reviewboard.mozilla.org/r/59438/#review56610

This patch comes from bug 1201363. It already be fixed. I think this comment is previous one. But I need to deal with forward declation for nested enum in class.
https://reviewboard.mozilla.org/r/59438/#review57624

> -> cpp

Per talk on IRC with Andreas, MediaEncoder inherite MediaStreamListener. So we need the header file. Another solution will be not to inherit but to have a member instead. We will let those task in another day.
https://reviewboard.mozilla.org/r/59438/#review57624

> It would be splendid if we could put these in the cpp as well. It shouldn't be harder than moving definitions that depend on them to the cpp.

VideoSegment.h will be removed in stop-buffering bug. We need this because VideoFrame MediaStream::mLastPlayedVideoFrame need it. And this is used in |PlayVideo|. That will be much easier if we remove VideoSegment.h in that bug since the |PlayVideo| will be replaced with VideoFrameContainer::SetCurrentFrames.
(In reply to Karl Tomlinson (ni?:karlt) from comment #65)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #60)
> > > +// Utility functions for transform between enum class and the underlying value.
> 
> I didn't look too closely at this but mfbt has EnumSet, which might be what
> you want.

A quick look on EnumSet, looks like it is designed for keeping enum as a member and manipulate it. And what we used right here is pass as argument. For example, see [1]. It still need a enum SeaBird for EnumSet. But the problem still here, how to forward declare a nested enum.

Anyway, enum can be forward declaration in C++11(Forward declarations for enums N2764) but not nested enum. So I change to use enum and no need of ToEnum and EnumValue.

Thanks KarlT.

[1]: https://dxr.mozilla.org/mozilla-central/source/mfbt/tests/TestEnumSet.cpp
https://reviewboard.mozilla.org/r/59438/#review57624

> Why do we need these?
> 
> And if we really do, they seem generic enough to go into mfbt/ or some such.

C++11 supports "Forward declarations for enums 	N2764". Adopt the new syntax and no need those function now.

> I guess making this an enum class is what is causing all the headache. I'm fine with leaving it as a regular enum inside MediaStreamListener (i.e., same as before) since it's a flag set.
> 
> Sure you lose some type safety but we're not regressing anything either.

We still can use nested enum for forward declaration but only enum. So I change to use enum in namespace.

> Why change scope of these events? I think the point was to keep MediaStreamListener events within the MediaStreamListener scope.

Those change are for build pass for enum class. But I would like to keep them since now the enum will not be nested. There is some potentil risk for naming conflict.

> Move to cpp.

Done, with moving GetUserMediaCallbackMediaStreamListener and typedef of "media::Pledge<bool, dom::MediaStreamError*>" outside class and renaming as GUMMSListenerPledgeVoid because of no support of nested typedef in forward declaration.

> This one seems off now.

Not sure what you mean, can you explain more?
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59438/diff/2-3/
Attachment #8763115 - Flags: review?(pehrson)
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/7-8/
https://reviewboard.mozilla.org/r/59438/#review58088

Looking pretty good now. I'll have a look on the full rev 3 before giving my final verdict.

::: dom/media/MediaManager.h:58
(Diff revisions 2 - 3)
> +class GetUserMediaCallbackMediaStreamListener;
> +class MediaManager;
> +class GetUserMediaTask;

Move these above GetMediaManagerLog() where we have other forward declarations.

::: dom/media/MediaManager.h:61
(Diff revisions 2 - 3)
> +// We can't use nested typedef in forward declaration.
> +typedef media::Pledge<bool, dom::MediaStreamError*> GUMMSListenerPledgeVoid;

If you move this to line 260 where we have other pledge typedefs, you can't use it in the listener, correct? Perhaps we should skip the typedef then. I don't want it outside a class.

If you keep it, call it PledgeBool per the pattern of the other typedefs.

::: dom/media/MediaManager.h:326
(Diff revisions 2 - 3)
>  
>    static StaticRefPtr<MediaManager> sSingleton;
>  
>    media::CoatCheck<PledgeSourceSet> mOutstandingPledges;
>    media::CoatCheck<PledgeChar> mOutstandingCharPledges;
> -  media::CoatCheck<GetUserMediaCallbackMediaStreamListener::PledgeVoid> mOutstandingVoidPledges;
> +  media::CoatCheck<GUMMSListenerPledgeVoid> mOutstandingVoidPledges;

s/Void/Bool/

::: dom/media/MediaStreamGraph.cpp:193
(Diff revisions 2 - 3)
>        aStream->ApplyTrackDisabling(data->mID, data->mData);
> -      StreamTime offset = (data->mCommands & EnumValue(TrackEventCommand::TRACK_EVENT_CREATED))
> +      StreamTime offset = (data->mCommands & TrackEventCommand::TRACK_EVENT_CREATED)
>            ? data->mStart : aStream->mTracks.FindTrack(data->mID)->GetSegment()->GetDuration();
>        for (MediaStreamListener* l : aStream->mListeners) {
>          l->NotifyQueuedTrackChanges(this, data->mID,
> -                                    offset, ToEnum<TrackEventCommand>(data->mCommands), *data->mData);
> +                                    offset, static_cast<TrackEventCommand>(data->mCommands), *data->mData);

Change TrackData::mCommands to be your enum type and you don't have to cast.
https://reviewboard.mozilla.org/r/59438/#review57624

> Not sure what you mean, can you explain more?

Indentation seems wrong.
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

https://reviewboard.mozilla.org/r/59438/#review58102

Almost there, but I'd like one last look before this goes in.

::: dom/media/MediaStreamGraph.h:16
(Diff revision 3)
>  
>  #include "mozilla/dom/AudioChannelBinding.h"
>  
> -#include "AudioSegment.h"
>  #include "AudioStream.h"
> +#include "VideoSegment.h"

Move back to line 21.

::: dom/media/MediaStreamGraph.h:792
(Diff revision 3)
> -  enum TrackCommands {
> -    TRACK_CREATE = MediaStreamListener::TRACK_EVENT_CREATED,
> +  virtual ~SourceMediaStream();
> +
> -    TRACK_END = MediaStreamListener::TRACK_EVENT_ENDED
> -  };

Keep the TrackCommands enum. It's equal to the track events now but doesn't necessarily stay that way.

You can make it explicit to not have to depend on the track events definition, like `TRACK_CREATE = 0x01`.

::: dom/media/MediaStreamListener.h:23
(Diff revision 3)
> +enum TrackEventCommand : uint32_t {
> +  TRACK_EVENT_NONE = 0x00,
> +  TRACK_EVENT_CREATED = 0x01,
> +  TRACK_EVENT_ENDED = 0x02,
> +  TRACK_EVENT_UNUSED = ~(TRACK_EVENT_ENDED | TRACK_EVENT_CREATED),

Call the enum TrackEvent instead.

Are TRACK_EVENT_NONE and TRACK_EVENT_UNUSED needed? If not I'm happy to see us keep only TRACK_EVENT_CREATED and TRACK_EVENT_ENDED like before.
Attachment #8763115 - Flags: review?(pehrson)
https://reviewboard.mozilla.org/r/59438/#review58102

> Call the enum TrackEvent instead.
> 
> Are TRACK_EVENT_NONE and TRACK_EVENT_UNUSED needed? If not I'm happy to see us keep only TRACK_EVENT_CREATED and TRACK_EVENT_ENDED like before.

The TRACK_EVENT_UNUSED is for ASSERT checking. And I think TRACK_EVENT_NONE is better than put some magic number 0 in the arguments of |NotifyQueuedTrackChanges|. The TRACK_EVENT_NONE might be able to removed eventually after stop buffering bug. I will do that if possible and I think it is possible.
https://reviewboard.mozilla.org/r/59438/#review58088

> Change TrackData::mCommands to be your enum type and you don't have to cast.

If we change TrackData::mCommands to enum type then it will cause build failure when you try to do bitwise operation. Because the bitwise operation will cause the original enum type doing impilict conversatio to uint32. And you need a static_cast conversation to turn it back to enum class. See below test code:


using namespace std;

enum E : uint32_t
{
  TEST_1 = 1,
  TEST_2,
};

E t1;
uint32_t t2;

int main()
{
  t2 = TEST_1;
  t2 |= TEST_2;
  
  t1 = TEST_1;
//  t1 |= TEST_2; // Build failure
  t1 = static_cast<E>(t1|TEST_2); // Build pass
  cout <<"t2 = " << t2 << endl;
}
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59438/diff/3-4/
Attachment #8763115 - Flags: review?(pehrson)
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/8-9/
Attachment #8763115 - Flags: review?(pehrson) → review+
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

https://reviewboard.mozilla.org/r/59438/#review58350
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/7-8/
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59438/diff/4-5/
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/9-10/
failed to apply:

applying 9328f6829b71
patching file dom/html/HTMLMediaElement.cpp
Hunk #4 FAILED at 770
1 out of 12 hunks FAILED -- saving rejects to file dom/html/HTMLMediaElement.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

can you take a look, thanks!
Flags: needinfo?(ctai)
Comment on attachment 8751099 [details]
Bug 1266646 - Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51807/diff/8-9/
Comment on attachment 8763115 [details]
Bug 1266646 - Move group of MediaStreamListener to a new header file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59438/diff/5-6/
Comment on attachment 8751098 [details]
Bug 1266646 - Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51805/diff/10-11/
(In reply to Carsten Book [:Tomcat] from comment #86)
> failed to apply:
> 
> applying 9328f6829b71
> patching file dom/html/HTMLMediaElement.cpp
> Hunk #4 FAILED at 770
> 1 out of 12 hunks FAILED -- saving rejects to file
> dom/html/HTMLMediaElement.cpp.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue
> 
> can you take a look, thanks!

I just updated the review board and rebased it on top of inbound.
Please check it in again.
Flags: needinfo?(ctai)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d77a9ed241ef
Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/6062a5e09ab7
Move group of MediaStreamListener to a new header file. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/49295086e189
Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener. r=pehrsons
Keywords: checkin-needed
Depends on: 1285060
Depends on: 1269741
No longer depends on: 1285060
No longer depends on: 1269741
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4045b08490e2
Disable assert temporarily to not cause too much pain to sherrifs.
(In reply to Pulsebot from comment #93)
> Pushed by paul@paul.cx:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4045b08490e2
> Disable assert temporarily to not cause too much pain to sherrifs.
Hi, Paul,
Thanks for the help!
Depends on: 1299172
Depends on: 1342949
Depends on: 1342950
You need to log in before you can comment on or make changes to this bug.