1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index.

Status: RESOLVED FIXED

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

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

MediaTrackList::RemoveTrack/AddTrack should be virtual and override by VideoTrackList::RemoveTrack/AddTrack. Because VideoTrackList::mSelectedIndex should be reflected when the MediaTrackList::mTracks changed.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug1271566.diff

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

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

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

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

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Add early return when selected track removed in |RemoveTrack|.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #3)
> Comment on attachment 8751086 [details]
> MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should
> also update selected index. r?jesup,pehrsons
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/51801/diff/1-2/

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

https://reviewboard.mozilla.org/r/51801/#review48777

::: dom/media/VideoTrackList.h:53
(Diff revision 2)
>  
>  protected:
>    VideoTrackList* AsVideoTrackList() override { return this; }
>  
>  private:
> +  void UpdateSelectedIndexByVideoTrack(VideoTrack* aTrack) ;

From the name it's not clear to me what this does.

::: dom/media/VideoTrackList.cpp:40
(Diff revision 2)
> +void
> +VideoTrackList::AddTrack(MediaTrack* aTrack)
> +{
> +  bool found;
> +  VideoTrack* videoTrack = IndexedGetter(mSelectedIndex, found);
> +  MediaTrackList::AddTrack(aTrack);
> +  UpdateSelectedIndexByVideoTrack(videoTrack);
> +}
> +
> +void
> +VideoTrackList::RemoveTrack(const RefPtr<MediaTrack>& aTrack)
> +{
> +  bool found;
> +  VideoTrack* videoTrack = IndexedGetter(mSelectedIndex, found);
> +  MediaTrackList::RemoveTrack(aTrack);
> +  if (aTrack == videoTrack) {
> +    mSelectedIndex = -1;
> +    return;
> +  }
> +  UpdateSelectedIndexByVideoTrack(videoTrack);
> +}

If I understand the semantics correctly here, why not inline the selection logic into AddTrack and RemoveTrack?

I think that makes it easier to read.

I.e.:

AddTrack(track):
if no selected track:
  selected track = track
  
RemoveTrack(track):
remove the track;
if track == selected track and there are still tracks:
  selected track = first remaining track;

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review48777

> If I understand the semantics correctly here, why not inline the selection logic into AddTrack and RemoveTrack?
> 
> I think that makes it easier to read.
> 
> I.e.:
> 
> AddTrack(track):
> if no selected track:
>   selected track = track
>   
> RemoveTrack(track):
> remove the track;
> if track == selected track and there are still tracks:
>   selected track = first remaining track;

Per talk with pehrsons in IRC, I change the selection logic in the new patch.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

https://reviewboard.mozilla.org/r/51801/#review49357

::: dom/media/VideoTrackList.cpp:32
(Diff revision 3)
> +  if (!(found && videoTrack && aTrack == videoTrack)) {
> +    mSelectedIndex = -1;
> +    return;
> +  }

Why set the selected index to -1 when the selected track was not the one removed?

Shouldn't it be:

```
if (mSelectedIndex == -1) {
  // There was no selected track and we don't select another track on removal.
  return;
}

MOZ_ASSERT(found, "When selectedIndex is set it should point to a track");
MOZ_ASSERT(videoTrack, "VideoTrackList shouldn't contain null tracks");

if (aTrack == videoTrack) {
  selectFirstTrack(); // pseudo
}

for loop to update the selected index in case the selected track moved;
```

::: dom/media/VideoTrackList.cpp:37
(Diff revision 3)
> +  for (size_t ix = 0; ix < mTracks.Length(); ix ++) {
> +    if (mTracks[ix] == aTrack) {
> +      mSelectedIndex = ix;
> +      return;
> +    }
> +  }
> +}

This should check `mTracks[ix] == videoTrack` for when videoTrack was found, but `videoTrack != aTrack`.

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review49357

> Why set the selected index to -1 when the selected track was not the one removed?
> 
> Shouldn't it be:
> 
> ```
> if (mSelectedIndex == -1) {
>   // There was no selected track and we don't select another track on removal.
>   return;
> }
> 
> MOZ_ASSERT(found, "When selectedIndex is set it should point to a track");
> MOZ_ASSERT(videoTrack, "VideoTrackList shouldn't contain null tracks");
> 
> if (aTrack == videoTrack) {
>   selectFirstTrack(); // pseudo
> }
> 
> for loop to update the selected index in case the selected track moved;
> ```

selectFirstTrack will be in bug Bug 1266646

Comment 10

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

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

https://reviewboard.mozilla.org/r/51801/#review49584

Similar comments to Andreas; I think one needs to be addressed, and you should explain in a comment here what bug 1266646 will resolve the first issue

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51801/diff/3-4/

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

https://reviewboard.mozilla.org/r/51801/#review49974

::: dom/media/VideoTrackList.cpp:29
(Diff revision 4)
> +  bool found;
> +  VideoTrack* videoTrack = IndexedGetter(mSelectedIndex, found);

Move these lines down to the asserts.

::: dom/media/VideoTrackList.cpp:39
(Diff revision 4)
> +  // This function also be called in |MediaDecoder::RemoveMediaTracks|. The
> +  // mSelectedIndex is not supported in such case. So just set the
> +  // mSelectedIndex to -1; The |HTMLMediaElement::NotifyMediaStreamTrackRemoved|
> +  // will handle the selection of the first remaining video track job.
> +  if (aTrack == videoTrack) {
> +    mSelectedIndex = -1;
> +    return;
> +  }

I don't understand the comment. mSelectedIndex is set when a track gets enabled per [1], no matter if we're playing a file or a stream.

(And if we're playing a file there'll only be 1 track per kind at most, but you shouldn't have to care here, right?)

[1] https://dxr.mozilla.org/mozilla-central/rev/a884b96685aa13b65601feddb24e5f85ba861561/dom/media/VideoTrack.cpp#66

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review49974

> Move these lines down to the asserts.

Per talk with Andreas on IRC. We can move those lines down. Because we need to find the video track before |MediaTrackList::RemoveTrack|.
And mSelectedIndex == -1 need to be done after RemoveTrack. Also the |MediaTrackList::RemoveTrack| is necessary even mSelectedIndex = -1.

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review49974

> I don't understand the comment. mSelectedIndex is set when a track gets enabled per [1], no matter if we're playing a file or a stream.
> 
> (And if we're playing a file there'll only be 1 track per kind at most, but you shouldn't have to care here, right?)
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/a884b96685aa13b65601feddb24e5f85ba861561/dom/media/VideoTrack.cpp#66

Yes...you are right.

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review49974

> Per talk with Andreas on IRC. We can move those lines down. Because we need to find the video track before |MediaTrackList::RemoveTrack|.
> And mSelectedIndex == -1 need to be done after RemoveTrack. Also the |MediaTrackList::RemoveTrack| is necessary even mSelectedIndex = -1.

Typo... We can not move those lines down.

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51801/diff/4-5/

Comment 17

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

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

https://reviewboard.mozilla.org/r/51801/#review50538

::: dom/media/VideoTrackList.cpp:43
(Diff revision 5)
> +    return;
> +  }
> +  MOZ_ASSERT(found, "When mSelectedIndex is set it should point to a track");
> +  MOZ_ASSERT(videoTrack, "The mSelectedIndex should be set to video track only");
> +
> +  // Left the caller of RemoveTrack to deal with choosing the next selected

nit (wording): Let the caller of RemoveTrack deal with choosing the new selected track if it removes the currently-selected track.

::: dom/media/VideoTrackList.cpp:50
(Diff revision 5)
> +  if (aTrack == videoTrack) {
> +    mSelectedIndex = -1;
> +    return;
> +  }
> +
> +  // The removed track is not selected track and there is a selected video track

nit (wording): The removed track was not the selected track and there is a currently-selected video track.

::: dom/media/VideoTrackList.cpp:53
(Diff revision 5)
> +  }
> +
> +  // The removed track is not selected track and there is a selected video track
> +  // before removing track. We need to find the new location of the selected
> +  // track.
> +  for (size_t ix = 0; ix < mTracks.Length(); ix ++) {

Perhaps switch to c++11-style iteration; otherwise remove space between ix and ++

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/51801/#review50538

> Perhaps switch to c++11-style iteration; otherwise remove space between ix and ++

We still need a local variable to memorize the index when switching to c++11-style iteration.
So I choose to remove space.

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51801/diff/5-6/

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8751086 [details]
MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51801/diff/6-7/

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb749a28eee0

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb749a28eee0