Closed
Bug 1266647
Opened 8 years ago
Closed 8 years ago
Clean NotifyQueuedTrackChange to only notify when command is track create and track end.
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, 5 obsolete files)
Right now, NotifyQueuedTrackChange do too many tasks. We should make it only notify when the commands is set to TRACK_END and TRACK_CREATE.
Comment 1•8 years ago
|
||
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #0) > Right now, NotifyQueuedTrackChange do too many tasks. We should make it only > notify when the commands is set to TRACK_END and TRACK_CREATE. While I don't disagree, it would help to mention what notifications you're talking about eliminating
Rank: 25
Flags: needinfo?(ctai)
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
Hi, Randell, Thanks for the question. Right now, there is only two track commands in the enum TrackCommands, TRACK_END and TRACK_CREATE. I don't want to eliminate those two commands. What I want to eliminate is no command case. I would like to add a new function called NotifyQueuedAudioData for audio case and leave NotifyQueuedTrackChange for TRACK_END and TRACK_CREATE only. For video case, stop-buffering bug will eliminate the need of NotifyQueuedVideoData by providing SetCurrentFrame in MediaStreamVideoSink. Please feel free to ni me or ping me in irc if you have any consideration. :)
Flags: needinfo?(ctai)
Assignee | ||
Comment 3•8 years ago
|
||
Another reason why I would like to make NotifyQueuedTrackChange only notify in those two commands is the TrackUnionStream will not copy the input of video track data to itself after stop-buffering landed. That means we can get the video data by NotifyQueuedTrackChange in TrackUnionStream.
Comment 4•8 years ago
|
||
This all sounds good, except the last sentence confused me: "That means we can get the video data by NotifyQueuedTrackChange in TrackUnionStream."
Assignee | ||
Comment 5•8 years ago
|
||
Oops, typo. Should be we can't get .... (In reply to Randell Jesup [:jesup] from comment #4) > This all sounds good, except the last sentence confused me: "That means we > can get the video data by NotifyQueuedTrackChange in TrackUnionStream."
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8746916 -
Attachment description: bug126646-1.diff → Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.
Attachment #8746916 -
Flags: review?(rjesup)
Assignee | ||
Updated•8 years ago
|
Attachment #8746916 -
Flags: review?(pehrsons)
Assignee | ||
Updated•8 years ago
|
Attachment #8746917 -
Attachment description: bug126646-2.diff → Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.
Attachment #8746917 -
Flags: review?(rjesup)
Assignee | ||
Updated•8 years ago
|
Attachment #8746917 -
Flags: review?(pehrsons)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8746916 [details] [diff] [review] Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener. Sorry...cancel review due to upload to wrong bug page. :(
Attachment #8746916 -
Flags: review?(rjesup)
Attachment #8746916 -
Flags: review?(pehrsons)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8746917 [details] [diff] [review] Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively. Sorry...cancel review due to upload to wrong bug page. :(
Attachment #8746917 -
Flags: review?(rjesup)
Attachment #8746917 -
Flags: review?(pehrsons)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8746916 -
Attachment is obsolete: true
Attachment #8746917 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8750648 -
Attachment is obsolete: true
Attachment #8750707 -
Flags: feedback?(pehrsons)
Comment 12•8 years ago
|
||
Comment on attachment 8750707 [details] [diff] [review] bug1266647.diff Review of attachment 8750707 [details] [diff] [review]: ----------------------------------------------------------------- We chatted about this on IRC.
Attachment #8750707 -
Flags: feedback?(pehrsons)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8750707 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52387/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52387/
Attachment #8752020 -
Flags: review?(rjesup)
Attachment #8752020 -
Flags: review?(pehrsons)
Comment 15•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons https://reviewboard.mozilla.org/r/52387/#review49365 ::: dom/media/MediaStreamGraph.cpp:195 (Diff revision 1) > + // The data->mCommands should be less than the sum of all command value. > + // Since the command is a series of the power of 2. The sum of value > + // should be 2^(n + 1) - 1; > + MOZ_ASSERT(data->mCommands < (SourceMediaStream::TRACK_COMMAND_END - 1) * > + 2 - 1); This is a bit hard to read and verify. Perhaps make it explicit instead? We're not likely to add commands I guess, but I could also see a utility function IsValidTrackCommand which checks them explicitly if we need it for reuse. ::: dom/media/MediaStreamGraph.cpp:200 (Diff revision 1) > + // The data->mCommands should be less than the sum of all command value. > + // Since the command is a series of the power of 2. The sum of value > + // should be 2^(n + 1) - 1; > + MOZ_ASSERT(data->mCommands < (SourceMediaStream::TRACK_COMMAND_END - 1) * > + 2 - 1); > + MOZ_ASSERT(!data->mData); This will fail in some cases. ::: dom/media/MediaStreamGraph.cpp:202 (Diff revision 1) > + if (data->mCommands & SourceMediaStream::TRACK_END) { > + l->NotifyQueuedAudioData(this, data->mID, > + offset, *(static_cast<AudioSegment*>(data->mData.get()))); > + } > + l->NotifyQueuedTrackChanges(this, data->mID, > + offset, data->mCommands, *data->mData); > + if (data->mCommands & SourceMediaStream::TRACK_CREATE) { > + l->NotifyQueuedAudioData(this, data->mID, > + offset, *(static_cast<AudioSegment*>(data->mData.get()))); > + } You don't need to check for TRACK_CREATE or TRACK_END here since they anyway cover all the cases. Make sure you check for audio though, and perhaps skip NotifyQueuedAudioData if the segment is null or contains nothing (duration, not data). ::: dom/media/TrackUnionStream.cpp:327 (Diff revision 1) > + // Separate Audio and Video. > + if (segment->GetType() == MediaSegment::AUDIO) { > + l->NotifyQueuedAudioData(Graph(), outputTrack->GetID(), > + outputStart, *static_cast<AudioSegment*>(segment)); > + } else { Verify that the TRACK_CREATE and TRACK_END cases for TrackUnionStream only pass along dummy segments. ::: dom/media/encoder/MediaEncoder.h:133 (Diff revision 1) > - * Notified the stream is being removed. > + * Notifed by the control loop of MediaStreamGraph; aQueueMedia is the raw > + * track data in the form of an AudioSegment. "raw" is a bit misguiding here since we call segments in a direct listener that have not been resampled "raw". Perhaps s/the raw track data/the audio data/? ::: dom/media/encoder/MediaEncoder.h:140 (Diff revision 1) > + */ > + void NotifyQueuedAudioData(MediaStreamGraph* aGraph, TrackID aID, > + StreamTime aTrackOffset, > + const AudioSegment& aQueuedMedia, > + MediaStream* aInputStream = nullptr, > + TrackID aInputTrackID = TRACK_INVALID) override; No default values in the override. ::: dom/media/encoder/MediaEncoder.cpp:115 (Diff revision 1) > + mAudioEncoder->NotifyQueuedTrackChanges(aGraph, aID, > + aTrackOffset, 0, > + aQueuedMedia); Looks like this could fit on fewer lines. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1986 (Diff revision 1) > + void NotifyQueuedAudioData(MediaStreamGraph* aGraph, mozilla::TrackID aID, > + StreamTime aTrackOffset, > + const AudioSegment& aQueuedMedia, > + MediaStream* aInputStream, > + TrackID aInputTrackID) override {} No need to add an override that does nothing (same as the one you're overriding). I wonder why the NotifyQueuedTrackChanges is there... ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2156 (Diff revision 1) > + void NotifyQueuedAudioData(MediaStreamGraph* aGraph, mozilla::TrackID aID, > + StreamTime aTrackOffset, > + const AudioSegment& aQueuedMedia, > + MediaStream* aInputStream, > + TrackID aInputTrackID) override {} Same here, this is not needed.
Attachment #8752020 -
Flags: review?(pehrsons)
Updated•8 years ago
|
Attachment #8752020 -
Flags: review?(rjesup) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons https://reviewboard.mozilla.org/r/52387/#review49481 r+ with nits; if we split video from command/events I'll want to see the followup ::: dom/media/MediaStreamGraph.h:162 (Diff revision 1) > + // Should be a power of two. > enum { > TRACK_EVENT_CREATED = 0x01, > - TRACK_EVENT_ENDED = 0x02 > + TRACK_EVENT_ENDED = 0x02, // maskable flags, not a simple enumerated value enumerated flags are a common pattern, so I don't need to do much here to verify them, just make sure it's noted ::: dom/media/MediaStreamGraph.h:176 (Diff revision 1) > virtual void NotifyQueuedTrackChanges(MediaStreamGraph* aGraph, TrackID aID, > StreamTime aTrackOffset, > uint32_t aTrackEvents, Should we just cleanly split things and move trackEvents to a separate method from video (and audio), or will this be handled in a following patch? ::: dom/media/MediaStreamGraph.cpp:195 (Diff revision 1) > + // The data->mCommands should be less than the sum of all command value. > + // Since the command is a series of the power of 2. The sum of value > + // should be 2^(n + 1) - 1; > + MOZ_ASSERT(data->mCommands < (SourceMediaStream::TRACK_COMMAND_END - 1) * > + 2 - 1); If you want to do validation here, include a TRACK_EVENT_UNUSED = ~(TRACK_EVENT_END | TRACK_EVENT_CREATED) in the enum, and use MOZ_ASSERT(!(data->mCommands & TRACK_EVENT_UNUSED)) ::: dom/media/MediaStreamGraph.cpp:203 (Diff revision 1) > + l->NotifyQueuedAudioData(this, data->mID, > + offset, *(static_cast<AudioSegment*>(data->mData.get()))); indent, here and below
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/52387/#review49481 > Should we just cleanly split things and move trackEvents to a separate method from video (and audio), or will this be handled in a following patch? The seperate method from video will be done in bug 1201363 (stop-buffering in MSG). After that, I would like to remove aQueuedMedia from |NotifyQueuedTrackChanges|. So the video data will be sent through below flow. SourceMediaStream::AppendToTrack -> MediaStreamVideoSink::SetCurrentFrame Each sink can receive video data from different thread by this way.
Assignee | ||
Comment 18•8 years ago
|
||
https://reviewboard.mozilla.org/r/52387/#review49365 > No need to add an override that does nothing (same as the one you're overriding). I wonder why the NotifyQueuedTrackChanges is there... Both are for FakeMediaStreams. Remove those line will cause build errors. FakeMediaStreams.h:90:16: note: unimplemented pure virtual method 'NotifyQueuedTrackChanges' in 'PipelineListener'
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52387/diff/1-2/
Attachment #8752020 -
Flags: review?(pehrsons)
Comment 20•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons https://reviewboard.mozilla.org/r/52387/#review49712 Almost there, but I want the event ordering to be solid before r+. ::: dom/media/MediaStreamGraph.cpp:197 (Diff revision 2) > + if (data->mData->GetType() == MediaSegment::AUDIO) { > + l->NotifyQueuedAudioData(this, data->mID, > + offset, *(static_cast<AudioSegment*>(data->mData.get()))); > + } Combine this... ::: dom/media/MediaStreamGraph.cpp:204 (Diff revision 2) > + } else if (data->mData->GetType() == MediaSegment::AUDIO) { > + for (MediaStreamListener* l : aStream->mListeners) { > + l->NotifyQueuedAudioData(this, data->mID, > + offset, *(static_cast<AudioSegment*>(data->mData.get()))); > + } ...with this and put it before or after the big if depending on the event order you need. Hmm, though it makes most sense to have TRACK_CREATE be issued first, then all the data events, then TRACK_END last, to not get things in the wrong order at the consumer..? Or make separate callbacks for TRACK_CREATE and TRACK_END since you're splitting things up anyway? I'm not sure what is best here, but the order could be important. ::: media/webrtc/signaling/test/FakeMediaStreams.h:97 (Diff revision 2) > + virtual void NotifyQueuedAudioData(mozilla::MediaStreamGraph* aGraph, mozilla::TrackID aID, > + mozilla::StreamTime aTrackOffset, > + const mozilla::AudioSegment& aQueuedMedia, > + Fake_MediaStream* aInputStream, > + mozilla::TrackID aInputTrackID) = 0; Can we implement empty defaults here to avoid having dummy impls in MediaPipeline?
Attachment #8752020 -
Flags: review?(pehrsons)
Assignee | ||
Updated•8 years ago
|
Attachment #8751155 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52387/diff/2-3/
Attachment #8752020 -
Flags: review?(pehrsons)
Comment 22•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons https://reviewboard.mozilla.org/r/52387/#review50648 I think you're issuing a bit too many events for audio. With that fixed, r+. ::: dom/media/MediaStreamGraph.cpp:219 (Diff revisions 2 - 3) > + // Video case. > + if (data->mCommands) { Shouldn't there be an `if (video)` thing here? Audio seems to fall through.
Attachment #8752020 -
Flags: review?(pehrsons) → review+
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/52387/#review50648 Thanks!
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8752020 [details] MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52387/diff/3-4/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 25•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/50c739a2cf3f
Keywords: checkin-needed
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50c739a2cf3f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•