Clean NotifyQueuedTrackChange to only notify when command is track create and track end.

RESOLVED FIXED in Firefox 49

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Right now, NotifyQueuedTrackChange do too many tasks. We should make it only notify when the commands is set to TRACK_END and TRACK_CREATE.
Assignee: nobody → ctai
Blocks: 1201363
(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
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)
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.
This all sounds good, except the last sentence confused me: "That means we can get the video data by NotifyQueuedTrackChange in TrackUnionStream."
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."
Created attachment 8746916 [details] [diff] [review]
Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.
Created attachment 8746917 [details] [diff] [review]
Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.
Attachment #8746916 - Attachment description: bug126646-1.diff → Change HTMLMediaElement::StreamSizeListerner to inherit MediaStreamTrackDirectListener.
Attachment #8746916 - Flags: review?(rjesup)
Attachment #8746916 - Flags: review?(pehrsons)
Attachment #8746917 - Attachment description: bug126646-2.diff → Rename MediaStreamDirectListener and MediaStreamTrackDirectListener to DirectMediaStreamListener and DirectMediaStreamTrackListener respectively.
Attachment #8746917 - Flags: review?(rjesup)
Attachment #8746917 - Flags: review?(pehrsons)
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)
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)
Created attachment 8750648 [details] [diff] [review]
bug1266647.diff
Attachment #8746916 - Attachment is obsolete: true
Attachment #8746917 - Attachment is obsolete: true
Created attachment 8750707 [details] [diff] [review]
bug1266647.diff
Attachment #8750648 - Attachment is obsolete: true
Attachment #8750707 - Flags: feedback?(pehrsons)
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)
Created attachment 8752020 [details]
MozReview Request: Bug 1266647 - Clean NotifyQueuedTrackChange to only notify when command is track create and track end. r=jesup,pehrsons

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 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

3 years ago
Attachment #8752020 - Flags: review?(rjesup) → review+
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
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.
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'
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 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)
Attachment #8751155 - Attachment is obsolete: true
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 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+
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/
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/50c739a2cf3f
Status: NEW → RESOLVED
Last Resolved: 2 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.