Closed
Bug 1061046
Opened 10 years ago
Closed 10 years ago
Eliminate TrackRate and TrackTicks
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(23 files, 6 obsolete files)
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.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2b0759fad712
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5131b580afd1 looks better.
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b4cd7a384101
Assignee | ||
Comment 4•10 years ago
|
||
Looks good, I don't think those failures are mine.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8492514 -
Flags: review?(karlt)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8492516 -
Flags: review?(karlt)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8492517 -
Flags: review?(karlt)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8492518 -
Flags: review?(karlt)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8492519 -
Flags: review?(karlt)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8492520 -
Flags: review?(karlt)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8492521 -
Flags: review?(karlt)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8492522 -
Flags: review?(karlt)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8492523 -
Flags: review?(karlt)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8492524 -
Flags: review?(karlt)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8492525 -
Flags: review?(karlt)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8492526 -
Flags: review?(karlt)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8492527 -
Flags: review?(karlt)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8492528 -
Flags: review?(karlt)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8492529 -
Flags: review?(karlt)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8492530 -
Flags: review?(karlt)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8492531 -
Flags: review?(karlt)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8492532 -
Flags: review?(karlt)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8492533 -
Flags: review?(karlt)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8492534 -
Flags: review?(karlt)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8492535 -
Flags: review?(karlt)
Assignee | ||
Comment 27•10 years ago
|
||
Note Part 21 intentionally skipped, sorry...
Updated•10 years ago
|
Attachment #8492516 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492515 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492522 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492535 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492524 -
Flags: review?(karlt) → review+
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
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 30•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8492514 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492517 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492518 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492519 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492529 -
Flags: review?(karlt) → review+
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8492528 -
Flags: review?(karlt) → review+
Comment 33•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8492532 -
Flags: review?(karlt) → review+
Comment 34•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8492531 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8492534 -
Flags: review?(karlt) → review+
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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 37•10 years ago
|
||
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 38•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: leave-open
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8492520 -
Attachment is obsolete: true
Attachment #8524274 -
Flags: review?(karlt)
Comment 41•10 years ago
|
||
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+
Assignee | ||
Comment 42•10 years ago
|
||
(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.
Assignee | ||
Comment 43•10 years ago
|
||
(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.
Comment 44•10 years ago
|
||
(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.
Assignee | ||
Comment 45•10 years ago
|
||
(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.
Assignee | ||
Comment 46•10 years ago
|
||
(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.
Assignee | ||
Comment 47•10 years ago
|
||
(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.
Assignee | ||
Comment 48•10 years ago
|
||
> > > 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.
Assignee | ||
Comment 49•10 years ago
|
||
Since resampling is not needed, we can preserve the number of input channels in each input audio block.
Assignee | ||
Updated•10 years ago
|
Attachment #8492526 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8525246 -
Flags: review?(karlt)
Assignee | ||
Comment 50•10 years ago
|
||
Also removes some other unnecessary rate conversions.
Attachment #8525249 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8492527 -
Attachment is obsolete: true
Assignee | ||
Comment 51•10 years ago
|
||
Attachment #8525250 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8492529 -
Attachment is obsolete: true
Assignee | ||
Comment 52•10 years ago
|
||
Attachment #8525251 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8492530 -
Attachment is obsolete: true
Assignee | ||
Comment 53•10 years ago
|
||
Attachment #8525252 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8492533 -
Attachment is obsolete: true
Assignee | ||
Comment 54•10 years ago
|
||
Attachment #8525253 -
Flags: review?(karlt)
Assignee | ||
Comment 55•10 years ago
|
||
Surprisingly, at first glance tests do not seem to have regressed: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=731125b76d43
Updated•10 years ago
|
Attachment #8525246 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8525253 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8525249 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8525251 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8525250 -
Flags: review?(karlt) → review+
Comment 56•10 years ago
|
||
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+
Assignee | ||
Comment 57•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9707a99cb32 https://hg.mozilla.org/integration/mozilla-inbound/rev/943f95aa986a https://hg.mozilla.org/integration/mozilla-inbound/rev/af358b9f4c39 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2ea9cfc90c https://hg.mozilla.org/integration/mozilla-inbound/rev/293cf7053cc6 https://hg.mozilla.org/integration/mozilla-inbound/rev/00e5373b168b https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a92023aa14 https://hg.mozilla.org/integration/mozilla-inbound/rev/596a7b88afb7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7ac92094e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3565d8ea6d61 https://hg.mozilla.org/integration/mozilla-inbound/rev/eeda13df89b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/098a4f0cd0ae https://hg.mozilla.org/integration/mozilla-inbound/rev/b93cef4c7112 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7a1b6dd80e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/76152dbfc842 https://hg.mozilla.org/integration/mozilla-inbound/rev/442dfde6455a https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d415a669b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6c0bfb269f https://hg.mozilla.org/integration/mozilla-inbound/rev/62c91ba5efb2 https://hg.mozilla.org/integration/mozilla-inbound/rev/43a6e96a6614
https://hg.mozilla.org/mozilla-central/rev/8803f3f31a91 https://hg.mozilla.org/mozilla-central/rev/99f0e4fdb42c https://hg.mozilla.org/mozilla-central/rev/f9707a99cb32 https://hg.mozilla.org/mozilla-central/rev/943f95aa986a https://hg.mozilla.org/mozilla-central/rev/af358b9f4c39 https://hg.mozilla.org/mozilla-central/rev/ed2ea9cfc90c https://hg.mozilla.org/mozilla-central/rev/293cf7053cc6 https://hg.mozilla.org/mozilla-central/rev/00e5373b168b https://hg.mozilla.org/mozilla-central/rev/f5a92023aa14 https://hg.mozilla.org/mozilla-central/rev/596a7b88afb7 https://hg.mozilla.org/mozilla-central/rev/ed7ac92094e9 https://hg.mozilla.org/mozilla-central/rev/3565d8ea6d61 https://hg.mozilla.org/mozilla-central/rev/eeda13df89b4 https://hg.mozilla.org/mozilla-central/rev/098a4f0cd0ae https://hg.mozilla.org/mozilla-central/rev/b93cef4c7112 https://hg.mozilla.org/mozilla-central/rev/c7a1b6dd80e0 https://hg.mozilla.org/mozilla-central/rev/76152dbfc842 https://hg.mozilla.org/mozilla-central/rev/442dfde6455a https://hg.mozilla.org/mozilla-central/rev/c5d415a669b5 https://hg.mozilla.org/mozilla-central/rev/ee6c0bfb269f https://hg.mozilla.org/mozilla-central/rev/62c91ba5efb2 https://hg.mozilla.org/mozilla-central/rev/43a6e96a6614
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•