Closed
Bug 1271566
Opened 8 years ago
Closed 8 years ago
VideoTrackList::RemoveTrack/AddTrack should also update selected index.
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P2)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ctai, Assigned: ctai)
References
Details
Attachments
(1 file, 1 obsolete file)
MediaTrackList::RemoveTrack/AddTrack should be virtual and override by VideoTrackList::RemoveTrack/AddTrack. Because VideoTrackList::mSelectedIndex should be reflected when the MediaTrackList::mTracks changed.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51801/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51801/
Attachment #8751086 -
Flags: review?(rjesup)
Attachment #8751086 -
Flags: review?(pehrsons)
Assignee | ||
Updated•8 years ago
|
Attachment #8750673 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
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/
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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;
Attachment #8751086 -
Flags: review?(pehrsons)
Assignee | ||
Comment 6•8 years ago
|
||
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.
Updated•8 years ago
|
Rank: 23
Component: Audio/Video → Audio/Video: MediaStreamGraph
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Attachment #8751086 -
Flags: review?(pehrsons)
Assignee | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
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`.
Attachment #8751086 -
Flags: review?(pehrsons)
Assignee | ||
Comment 9•8 years ago
|
||
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•8 years ago
|
||
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
Attachment #8751086 -
Flags: review?(rjesup)
Assignee | ||
Comment 11•8 years ago
|
||
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/
Attachment #8751086 -
Flags: review?(rjesup)
Attachment #8751086 -
Flags: review?(pehrsons)
Updated•8 years ago
|
Attachment #8751086 -
Flags: review?(pehrsons) → review+
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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•8 years ago
|
||
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 ++
Attachment #8751086 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Attachment #8751086 -
Attachment description: MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r?jesup,pehrsons → MozReview Request: Bug 1271566 - VideoTrackList::RemoveTrack/AddTrack should also update selected index. r=jesup,pehrsons
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb749a28eee0
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb749a28eee0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•