Eliminate TrackRate and TrackTicks

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(23 attachments, 6 obsolete attachments)

4.75 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.23 KB, patch
karlt
: review+
karlt
: checkin+
Details | Diff | Splinter Review
4.26 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.32 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.04 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.53 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.91 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.14 KB, patch
karlt
: review+
Details | Diff | Splinter Review
16.35 KB, patch
karlt
: review+
Details | Diff | Splinter Review
4.88 KB, patch
karlt
: review+
Details | Diff | Splinter Review
39.04 KB, patch
karlt
: review+
Details | Diff | Splinter Review
4.87 KB, patch
karlt
: review+
Details | Diff | Splinter Review
13.44 KB, patch
karlt
: review+
Details | Diff | Splinter Review
6.97 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.79 KB, patch
karlt
: review+
Details | Diff | Splinter Review
2.31 KB, patch
karlt
: review+
Details | Diff | Splinter Review
6.20 KB, patch
karlt
: review+
Details | Diff | Splinter Review
25.83 KB, patch
karlt
: review+
Details | Diff | Splinter Review
13.08 KB, patch
karlt
: review+
Details | Diff | Splinter Review
15.42 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.10 KB, patch
karlt
: review+
Details | Diff | Splinter Review
122.95 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.27 KB, patch
karlt
: review+
Details | Diff | Splinter Review
As of now, all tracks in a MediaStreamGraph either have a TrackRate equal to the graph's audio rate, or a TrackRate of 1000000 (for video). We can make video frame durations also use the graph's audio rate, and then eliminate TrackRate and all the conversions between track rates and StreamTime, simplifying a bunch of code.
Looks good, I don't think those failures are mine.
This is guaranteed to work even if the stream has been disconnected from the graph,
so it's preferred to Graph()->GraphRate().
Attachment #8492515 - Flags: review?(karlt)
Note Part 21 intentionally skipped, sorry...
Attachment #8492516 - Flags: review?(karlt) → review+
Attachment #8492515 - Flags: review?(karlt) → review+
Attachment #8492522 - Flags: review?(karlt) → review+
Attachment #8492535 - Flags: review?(karlt) → review+
Attachment #8492524 - Flags: review?(karlt) → review+
Comment on attachment 8492523 [details] [diff] [review]
Part 9: Split SourceMediaStream::AddTrack into a method that adds an audio track and can resample, and a method that can add any track but always uses the graph rate

>+  data->mInputRate = aRate < 0 ? (graph ? graph->GraphRate() : 1) : aRate;

>+  void AddTrack(TrackID aID, TrackTicks aStart, MediaSegment* aSegment)
>+  {
>+    AddTrackInternal(aID, -1, aStart, aSegment);
>+  }

Perhaps pass GraphRate() instead of -1 here and then AddTrackInternal would be
simpler, but land what you have already, if you like.

>+        NS_ASSERTION(mStream->AsSourceStream()->GraphRate() == track_rate_,

mStream->GraphRate() is available.
Attachment #8492523 - Flags: review?(karlt) → review+
Comment on attachment 8492521 [details] [diff] [review]
Part 7: Remove MediaPipeline's USECS_PER_S video rate and use the graph rate instead

r+ on the MediaPipeline.cpp changes and Fake_MediaStreamGraph removal, but

>+#include "mozilla/UniquePtr.h"

please leave this out

>@@ -93,17 +94,17 @@ class Fake_MediaStream {
>   virtual void Periodic() {}
> 
>   double StreamTimeToSeconds(mozilla::StreamTime aTime);
>   mozilla::StreamTime
>   TicksToTimeRoundDown(mozilla::TrackRate aRate, mozilla::TrackTicks aTicks);
> 
>   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Fake_MediaStream);
> 
>- protected:
>+protected:

and leave this as is, for consistency with the rest of this class.
Attachment #8492521 - Flags: review?(karlt) → review+
Comment on attachment 8492525 [details] [diff] [review]
Part 11: Remove callback rate parameters

OK.  I was wondering why the MediaStreamGraph* parameter was here.
(I was expecting a MediaStream*, if anything.)

Some other MediaStreamListener methods use it for
DispatchToMainThreadAfterStreamStateUpdate(), so I guess
NotifyQueuedTrackChanges() is consistent.

>    * This will be called on any MediaStreamDirectListener added to a
>    * a SourceMediaStream when AppendToTrack() is called.  The MediaSegment
>    * will be the RawSegment (unresampled) if available in AppendToTrack().
>    * Note that NotifyQueuedTrackChanges() calls will also still occur.
>    */
>   virtual void NotifyRealtimeData(MediaStreamGraph* aGraph, TrackID aID,
>-                                  TrackRate aTrackRate,

This is the only documentation I can find for the aRawSegment parameter to
AppendToTrack().

The sample rate of the Segment passed to NotifyRealtimeData may or may not
have been resampled by the AppendToTrack, so I don't know what
NotifyRealtimeData's TrackRate parameter was meant to indicate.

But "(unresampled)" was here in
http://hg.mozilla.org/mozilla-central/rev/c55a56be94f2
before AppendToTrack did resampling, so I assume that the RawSegment has some
other sample rate that is neither the Track input or output rate anyway.

And I don't see any code that passes a non-null aRawSegment parameter.
Attachment #8492525 - Flags: review?(karlt) → review+
Attachment #8492514 - Flags: review?(karlt) → review+
Attachment #8492517 - Flags: review?(karlt) → review+
Attachment #8492518 - Flags: review?(karlt) → review+
Attachment #8492519 - Flags: review?(karlt) → review+
Attachment #8492529 - Flags: review?(karlt) → review+
Comment on attachment 8492520 [details] [diff] [review]
Part 6: Remove MediaDecoderStateMachine's USECS_PER_S video rate and use the graph rate instead

>+  StreamTime MicrosecondsToStreamTimeRoundDown(int64_t aMicroseconds) {
>+    return SecondsToStreamTimeRoundDown(aMicroseconds/1000000.0);
>+  }

Please use integer arithmetic to make the rounding much more predictable.

>       endPosition = std::max(endPosition,
>-          mediaStream->TicksToTimeRoundDown(RATE_VIDEO, stream->mNextVideoTime - stream->mInitialTime));
>+          mediaStream->TicksToTimeRoundDown(
>+              mediaStream->GraphRate(),
>+              stream->mNextVideoTime - stream->mInitialTime));

mNextVideoTime and mInitialTime are microseconds, so I think there must be
something wrong here.
Attachment #8492520 - Flags: review?(karlt) → review-
Comment on attachment 8492530 [details] [diff] [review]
Part 16: Remove TicksToTimeRoundDown

(In reply to Karl Tomlinson (:karlt) from comment #31)
> mNextVideoTime and mInitialTime are microseconds, so I think there must be
> something wrong here.

Ah, that's fixed here.
Attachment #8492530 - Flags: review?(karlt) → review+
Attachment #8492528 - Flags: review?(karlt) → review+
Comment on attachment 8492526 [details] [diff] [review]
Part 12: Simplify AudioNodeExternalInputStream since no resampling is needed

Nice to get rid of lots of code here, thanks.

The behavior change of keeping the number of input channels is good too, and
worth mentioning in the commit message.

>+CopyChunkToBlock(const AudioChunk& aInput, AudioChunk *aBlock,

>+  nsTArray<const void*> channels;

I think it's worth keeping nsAutoTArray for this.

>-    // Create a TrackMapEntry if necessary.
>-    size_t trackMapIndex = GetTrackMapEntry(inputTrack, aFrom);
>-    // Maybe there's nothing in this track yet. If so, ignore it. (While the
>-    // track is only playing silence, we may not be able to determine the
>-    // correct number of channels to start resampling.)
>-    if (trackMapIndex == nsTArray<TrackMapEntry>::NoIndex) {
>-      continue;
>-    }
>-
>-    while (trackMapEntriesUsed.Length() <= trackMapIndex) {
>-      trackMapEntriesUsed.AppendElement(false);
>-    }
>-    trackMapEntriesUsed[trackMapIndex] = true;
>-
>-    TrackMapEntry* trackMap = &mTrackMap[trackMapIndex];
>-    AudioSegment segment;
>+    AudioSegment& segment = *audioSegments.AppendElement();

This is skipping the check for a null track.

audioSegments is an array with auto length 1,
but mChunks on the MediaSegment is not an auto array.

Would it worth skipping that memory allocation here?

> AudioNodeStream::AccumulateInputChunk(uint32_t aInputIndex, const AudioChunk& aChunk,
>                                       AudioChunk* aBlock,
>                                       nsTArray<float>* aDownmixBuffer)
> {
>   nsAutoTArray<const void*,GUESS_AUDIO_CHANNELS> channels;
>-  UpMixDownMixChunk(&aChunk, aBlock->mChannelData.Length(), channels, *aDownmixBuffer);
>+  UpMixDownMixChunk(&aChunk, aBlock->mChannelData.Length(), channels,
>+                    *aDownmixBuffer);
> 

No unrelated whitespace change please.

>   audio.oncanplaythrough = function() {
>-    // Use a fuzz factor of 100 to account for samples that just happen to be zero
>-    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 100;
>+    // Use a fuzz factor of 200 to account for samples that just happen to be zero
>+    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 200;
>     expectedMaxNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) + 500;

Why is this change necessary?
The offset of 1 is now removed, so I could understand decrease of max and min
by 1, but this is increasing the accepted variability by 100 samples.

It may seem like a small change given the existing fuzz, but it is the only
tests we have for resampling into the MSG, which has broken a couple of times,
so I'd like to keep this as tight as possible.

Is the first non-null segment not long enough to cover the block?

r- because I want to hear about the test change.

The observation below is for your consideration.

>-    NS_ASSERTION(!ci.IsEnded(), "Segment must have at least one chunk");
>-    AudioChunk& firstChunk = *ci;
>-    ci.Next();
>-    if (ci.IsEnded()) {
>-      *aBlock = firstChunk;
>+    NS_ASSERTION(!ci.IsEnded(), "Should be at least one chunk!");
>+    if (ci->GetDuration() == WEBAUDIO_BLOCK_SIZE &&
>+        (ci->IsNull() || ci->mBufferFormat == AUDIO_FORMAT_FLOAT32)) {
>+      // Return this chunk directly to avoid copying data.
>+      *aBlock = *ci;
>       return;
>     }
>-
>-    while (ci->IsNull() && !ci.IsEnded()) {
>-      ci.Next();
>-    }
>-    if (ci.IsEnded()) {
>-      // All null.
>-      aBlock->SetNull(WEBAUDIO_BLOCK_SIZE);
>-      return;
>-    }

The new code doesn't have a fast path for multiple null chunks.
I don't know how likely/common it is for something upstream to write many
non-block-aligned null chunks. 

I don't think it is certain that null chunks will be coalesced; it depends on
how they are added.

(The old code was making an assumption that the first chunk was null if
other chunks are null.)
Attachment #8492526 - Flags: review?(karlt) → review-
Attachment #8492532 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #33)
> Why is this change necessary?
> The offset of 1 is now removed, so I could understand decrease of max and min
> by 1,

Actually, no, the translation shouldn't change the length.
I don't know why there is a change here.
Attachment #8492531 - Flags: review?(karlt) → review+
Attachment #8492534 - Flags: review?(karlt) → review+
Comment on attachment 8492527 [details] [diff] [review]
Part 13: Remove rate-conversion functions from StreamBuffer and Track

This feels like two different sets of changes in one patch.  Within MSG and
StreamBuffer/Track, TrackTicks and StreamTime are now the same thing and so
clean-up there is all good.

However, outside MSG, there are a few cases where TrackRate and TrackTicks
comes from other rates and frame counts that get resampled when fed
into the MSG.  I'm less confident about the changes there.

>@@ -418,19 +418,19 @@ void MediaDecoderStateMachine::SendStrea

>+      TrackRate graphRate = mediaStream->GraphRate();
>       endPosition = std::max(endPosition,
>-          mediaStream->TicksToTimeRoundDown(mInfo.mAudio.mRate,
>-                                            stream->mAudioFramesWritten));
>+          (stream->mAudioFramesWritten*graphRate)/mInfo.mAudio.mRate);

I have mixed feelings on this.

RateConvertTicksRoundDown() had some sanity assertions, but hopefully we don't
need them.  That function is also awkward because of three parameters of compatible type and no obvious order, but having a conversion method on the stream reduced the number of parameters to two, mitigating this a bit.  TicksToTimeRoundDown() could be renamed if the concept of ticks is not what is wanted here.

I kinda liked having conversions in one place.  I guess the math to round down is not so complicated and so perhaps it is OK to do the conversions explicitly, as long as there is not too many of them.

>@@ -2969,18 +2966,17 @@ MediaStreamGraph::StartNonRealtimeProces

>-  graph->mEndTime = graph->IterationEnd() +
>-    RateConvertTicksRoundUp(graph->GraphRate(), aRate, aTicksToProcess);
>+  graph->mEndTime = graph->IterationEnd() + aTicksToProcess;

It would be nice to remove the aRate parameter here,
or assert that aRate == GraphRate().

>   int64_t StreamTimeToMicroseconds(StreamTime aTime)
>   {
>-    return TimeToTicksRoundDown(1000000, aTime);
>+    return int64_t(TrackTicksToSeconds(mBuffer.GraphRate(), aTime)*1000000);

I don't think it is helpful to introduce floating point uncertainties, and I
fear this could lead to hard to diagnose problems.

>   TrackTicks TimeToTicksRoundUp(TrackRate aRate, StreamTime aTime)
>   {
>-    return RateConvertTicksRoundUp(aRate, mBuffer.GraphRate(), aTime);
>+    return aTime;
>   }
>   TrackTicks TimeToTicksRoundDown(TrackRate aRate, StreamTime aTime)
>   {
>-    return RateConvertTicksRoundDown(aRate, mBuffer.GraphRate(), aTime);
>+    return aTime;
>   }
>   StreamTime TicksToTimeRoundDown(TrackRate aRate, TrackTicks aTicks)
>   {
>-    return RateConvertTicksRoundDown(mBuffer.GraphRate(), aRate, aTicks);
>+    return aTime;
>   }

This is OK, only because these methods are removed.
Some clients were using this for a TrackRate not matching the graph rate and
TrackTicks not matching StreamTime.

>   GraphTime MillisecondsToMediaTime(int32_t aMS)
>   {
>-    return RateConvertTicksRoundDown(GraphRate(), 1000, aMS);
>+    return SecondsToTicksRoundDown(GraphRate(), aMS/1000.0);
>   }

Again, no unnecessary floating point, please.

>   TrackTicks TimeToTicksRoundDown(TrackRate aRate, StreamTime aTime)
>   {
>-    return RateConvertTicksRoundDown(aRate, GraphRate(), aTime);
>+    return aTime;
>   }

This MediaStreamGraphImpl method is no longer used, so please just remove it.

>+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
>@@ -1304,18 +1304,19 @@ static void AddTrackAndListener(MediaStr

>+      TrackRate graph_rate = mStream->GraphRate();
>       TrackTicks current_ticks =
>-        mStream->TimeToTicksRoundUp(track_rate_, current_end);
>+          (current_end * track_rate_ + graph_rate - 1) / graph_rate;

For rounding up, at least, it seems it would be helpful to have a method to do
this as the math is not so obvious for those less familiar.
And that suggests having a similar RoundDown method to be consistent.
Attachment #8492527 - Flags: review?(karlt) → review-
Comment on attachment 8492527 [details] [diff] [review]
Part 13: Remove rate-conversion functions from StreamBuffer and Track

>@@ -1385,19 +1386,19 @@ MediaPipelineReceiveAudio::PipelineListe

>+  TrackTicks graph_rate = source_->GraphRate();

TrackRate
Comment on attachment 8492533 [details] [diff] [review]
Part 19: Eliminate TrackTicks in favour of StreamTime

>diff --git a/dom/media/MediaManager.h b/dom/media/MediaManager.h

>-  TrackTicks mLastEndTimeAudio;

>+  StreamTime mLastEndTimeAudio;

>@@ -1305,17 +1305,17 @@ static void AddTrackAndListener(MediaStr

>       TrackRate graph_rate = mStream->GraphRate();
>-      TrackTicks current_ticks =
>+      StreamTime current_ticks =
>           (current_end * track_rate_ + graph_rate - 1) / graph_rate;

>+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
>@@ -299,50 +299,50 @@ class GenericReceiveListener : public Me

>-  void SetPlayedTicks(TrackTicks time) {
>+  void SetPlayedTicks(StreamTime time) {
>     played_ticks_ = time;
>   }

>-  TrackTicks played_ticks_;
>+  StreamTime played_ticks_;

I haven't looked through all the changes here, but at least these places seem
to be instances where the tick count does not have the same rate as
StreamTime.

I guess you could argue that TrackTicks is also not based on the *Track* rate,
but a convention has developed that TrackTicks is used for rates outside of
MediaStreams.

Changing TrackTicks to StreamTime in the MSG, where it is a StreamTime is
probably helpful, but wouldn't it be helpful to maintain a distinction
between graph units and other tick counts? 

> void MediaPipelineReceiveAudio::PipelineListener::
> NotifyPull(MediaStreamGraph* graph, StreamTime desired_time) {
>   MOZ_ASSERT(source_);
>   if (!source_) {
>     MOZ_MTLOG(ML_ERROR, "NotifyPull() called from a non-SourceMediaStream");
>     return;
>   }
> 
>-  TrackTicks graph_rate = source_->GraphRate();
>+  StreamTime graph_rate = source_->GraphRate();

TrackRate still.
Attachment #8492533 - Flags: review?(karlt)
Comment on attachment 8492515 [details] [diff] [review]
Part 1.5: Expose MediaStream::GraphRate()

I landed this part for an assertion in the patch for bug 1083038.

https://hg.mozilla.org/integration/mozilla-inbound/rev/18d1b0c0bc96
Attachment #8492515 - Flags: checkin+
Keywords: leave-open
Keywords: leave-open
Posted patch Part 6 v2Splinter Review
Attachment #8492520 - Attachment is obsolete: true
Attachment #8524274 - Flags: review?(karlt)
Comment on attachment 8524274 [details] [diff] [review]
Part 6 v2

Can RATE_VIDEO still be removed?
Are you intentionally reinstating SecondsToStreamTimeRoundDown()?
(I don't remember whether other patches will need this.)
Attachment #8524274 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #41)
> Can RATE_VIDEO still be removed?

Yes. Fixed.

> Are you intentionally reinstating SecondsToStreamTimeRoundDown()?
> (I don't remember whether other patches will need this.)

No. Fixed.
(In reply to Karl Tomlinson (:karlt) from comment #33)
> This is skipping the check for a null track.

I'm not sure what you meant here.

> audioSegments is an array with auto length 1,
> but mChunks on the MediaSegment is not an auto array.
> 
> Would it worth skipping that memory allocation here?

I'm not sure what you meant here either.

> >   audio.oncanplaythrough = function() {
> >-    // Use a fuzz factor of 100 to account for samples that just happen to be zero
> >-    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 100;
> >+    // Use a fuzz factor of 200 to account for samples that just happen to be zero
> >+    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 200;
> >     expectedMaxNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) + 500;
> 
> Why is this change necessary?
> The offset of 1 is now removed, so I could understand decrease of max and min
> by 1, but this is increasing the accepted variability by 100 samples.
> 
> It may seem like a small change given the existing fuzz, but it is the only
> tests we have for resampling into the MSG, which has broken a couple of
> times,
> so I'd like to keep this as tight as possible.

I don't know... I'll look into it.

> Is the first non-null segment not long enough to cover the block?
> 
> r- because I want to hear about the test change.
> 
> The observation below is for your consideration.
...
> The new code doesn't have a fast path for multiple null chunks.
> I don't know how likely/common it is for something upstream to write many
> non-block-aligned null chunks. 
> 
> I don't think it is certain that null chunks will be coalesced; it depends on
> how they are added.
> 
> (The old code was making an assumption that the first chunk was null if
> other chunks are null.)

I don't think this matters much.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> (In reply to Karl Tomlinson (:karlt) from comment #33)
> > This is skipping the check for a null track.
> 
> I'm not sure what you meant here.
> 
> > audioSegments is an array with auto length 1,
> > but mChunks on the MediaSegment is not an auto array.
> > 
> > Would it worth skipping that memory allocation here?
> 
> I'm not sure what you meant here either.

Much of efficiency of the audio graph, particularly given the delays before
GC can remove obsolete streams, depends on processing null input efficiently.

In the old code, GetTrackMapEntry() would return NoIndex if a track didn't
have an entry and had only null chunks. i.e. it hadn't yet produced output.
That lead to the iteration being skipped and no audioSements entry for that
track.

An inputTrack.GetSegment()->IsNull() check could do something similar for this
new code, and would be better even because the fast path would also be taken
when the track is finished.

Fast paths may often not be worth the extra code, but it may be worth extra
code to skip a memory allocation.  Even a segment.AppendNullData() requires a
memory allocation.
(In reply to Karl Tomlinson (:karlt) from comment #35)
> RateConvertTicksRoundDown() had some sanity assertions, but hopefully we
> don't
> need them.  That function is also awkward because of three parameters of
> compatible type and no obvious order, but having a conversion method on the
> stream reduced the number of parameters to two, mitigating this a bit. 
> TicksToTimeRoundDown() could be renamed if the concept of ticks is not what
> is wanted here.
> 
> I kinda liked having conversions in one place.  I guess the math to round
> down is not so complicated and so perhaps it is OK to do the conversions
> explicitly, as long as there is not too many of them.

I'll move the conversion functions out to MediaSegment.h.

> >@@ -2969,18 +2966,17 @@ MediaStreamGraph::StartNonRealtimeProces
> 
> >-  graph->mEndTime = graph->IterationEnd() +
> >-    RateConvertTicksRoundUp(graph->GraphRate(), aRate, aTicksToProcess);
> >+  graph->mEndTime = graph->IterationEnd() + aTicksToProcess;
> 
> It would be nice to remove the aRate parameter here,
> or assert that aRate == GraphRate().

I'll do that in a separate patch.

> >   int64_t StreamTimeToMicroseconds(StreamTime aTime)
> >   {
> >-    return TimeToTicksRoundDown(1000000, aTime);
> >+    return int64_t(TrackTicksToSeconds(mBuffer.GraphRate(), aTime)*1000000);
> 
> I don't think it is helpful to introduce floating point uncertainties, and I
> fear this could lead to hard to diagnose problems.

OK

> >   GraphTime MillisecondsToMediaTime(int32_t aMS)
> >   {
> >-    return RateConvertTicksRoundDown(GraphRate(), 1000, aMS);
> >+    return SecondsToTicksRoundDown(GraphRate(), aMS/1000.0);
> >   }
> 
> Again, no unnecessary floating point, please.

OK

> >   TrackTicks TimeToTicksRoundDown(TrackRate aRate, StreamTime aTime)
> >   {
> >-    return RateConvertTicksRoundDown(aRate, GraphRate(), aTime);
> >+    return aTime;
> >   }
> 
> This MediaStreamGraphImpl method is no longer used, so please just remove it.

It's removed in a later patch.

> >+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> >@@ -1304,18 +1304,19 @@ static void AddTrackAndListener(MediaStr
> 
> >+      TrackRate graph_rate = mStream->GraphRate();
> >       TrackTicks current_ticks =
> >-        mStream->TimeToTicksRoundUp(track_rate_, current_end);
> >+          (current_end * track_rate_ + graph_rate - 1) / graph_rate;
> 
> For rounding up, at least, it seems it would be helpful to have a method to
> do
> this as the math is not so obvious for those less familiar.
> And that suggests having a similar RoundDown method to be consistent.

I'll put this in MediaSegment.h as well.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> I'll move the conversion functions out to MediaSegment.h.

Actually I decided to just not remove the existing conversion functions and keep using them.
(In reply to Karl Tomlinson (:karlt) from comment #37)
> >diff --git a/dom/media/MediaManager.h b/dom/media/MediaManager.h
> 
> >-  TrackTicks mLastEndTimeAudio;
> 
> >+  StreamTime mLastEndTimeAudio;

I believe these are all actual track times (at the graph rate) so this change is OK.

> >@@ -1305,17 +1305,17 @@ static void AddTrackAndListener(MediaStr
> 
> >       TrackRate graph_rate = mStream->GraphRate();
> >-      TrackTicks current_ticks =
> >+      StreamTime current_ticks =
> >           (current_end * track_rate_ + graph_rate - 1) / graph_rate;

Right. I'm dropping this change.

> >+++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> >@@ -299,50 +299,50 @@ class GenericReceiveListener : public Me
> 
> >-  void SetPlayedTicks(TrackTicks time) {
> >+  void SetPlayedTicks(StreamTime time) {
> >     played_ticks_ = time;
> >   }
> 
> >-  TrackTicks played_ticks_;
> >+  StreamTime played_ticks_;

Right, I'm dropping this too.

> Changing TrackTicks to StreamTime in the MSG, where it is a StreamTime is
> probably helpful, but wouldn't it be helpful to maintain a distinction
> between graph units and other tick counts?

Yeah OK. We still get rid of the vast majority of TrackTicks usage which I think is fine.
> > >   audio.oncanplaythrough = function() {
> > >-    // Use a fuzz factor of 100 to account for samples that just happen to be zero
> > >-    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 100;
> > >+    // Use a fuzz factor of 200 to account for samples that just happen to be zero
> > >+    expectedMinNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) - 200;
> > >     expectedMaxNonzeroSampleCount = Math.floor(audio.duration*context.sampleRate) + 500;
> > 
> > Why is this change necessary?
> > The offset of 1 is now removed, so I could understand decrease of max and min
> > by 1, but this is increasing the accepted variability by 100 samples.
> > 
> > It may seem like a small change given the existing fuzz, but it is the only
> > tests we have for resampling into the MSG, which has broken a couple of
> > times,
> > so I'd like to keep this as tight as possible.
> 
> I don't know... I'll look into it.

These changes do not appear to be necessary anymore. I've removed them.
Since resampling is not needed, we can preserve the number of input channels in each
input audio block.
Also removes some other unnecessary rate conversions.
Attachment #8525249 - Flags: review?(karlt)
Attachment #8525246 - Flags: review?(karlt) → review+
Attachment #8525253 - Flags: review?(karlt) → review+
Attachment #8525249 - Flags: review?(karlt) → review+
Attachment #8525251 - Flags: review?(karlt) → review+
Attachment #8525250 - Flags: review?(karlt) → review+
Comment on attachment 8525252 [details] [diff] [review]
Part 19: Eliminate TrackTicks in favour of StreamTime

--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h
+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h
@@ -58,17 +58,17 @@ public:
                             const MediaEnginePrefs &aPrefs) MOZ_OVERRIDE;
   virtual nsresult Deallocate() MOZ_OVERRIDE;
   virtual nsresult Start(SourceMediaStream* aStream, TrackID aID) MOZ_OVERRIDE;
   virtual nsresult Stop(SourceMediaStream* aSource, TrackID aID) MOZ_OVERRIDE;
   virtual void NotifyPull(MediaStreamGraph* aGraph,
                           SourceMediaStream* aSource,
                           TrackID aId,
                           StreamTime aDesiredTime,
-                          TrackTicks& aLastEndTime) MOZ_OVERRIDE;
+                          StreamTime &aLastEndTime) MOZ_OVERRIDE;

I believe the convention is "& ".

Similarly 2 times in MediaEngineWebRTC.h

Otherwise all good, thank you.
Attachment #8525252 - Flags: review?(karlt) → review+
Blocks: 983052
You need to log in before you can comment on or make changes to this bug.