Closed
Bug 1171760
Opened 9 years ago
Closed 9 years ago
Optimise TrackBuffersManager speed
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
1.04 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
768 bytes,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
41.85 KB,
patch
|
mozbugz
:
review+
|
Details | Diff | Splinter Review |
2.59 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
This is a meta-bug designed to track the required improvements to TrackBuffersManager and the new MSE architecture. The code was designed to be spec-exact, not efficient. It is extremely slow due to how it deals frame by frame and its use of vectors as memory backend which isn't ideal when performing lots of insertion / removal. I have various optimizations pending, but will wait until the current code is fully working before spending time optimizing it.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8627037 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8627038 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 3•9 years ago
|
||
Invalid error rather than not supported.
Attachment #8627039 -
Flags: review?(ajones)
Assignee | ||
Comment 4•9 years ago
|
||
This provides significant speed improvement, halving the CPU usage. Rationalise similar methods. This assumes that frames within a media segment are continuous.
Attachment #8627040 -
Flags: review?(gsquelart)
Assignee | ||
Comment 5•9 years ago
|
||
This allows for MediaSource log to be usable as displaying log samples is extremely verbose.
Attachment #8627041 -
Flags: review?(cpearce)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8627042 -
Flags: review?(cpearce)
Updated•9 years ago
|
Attachment #8627037 -
Flags: review?(matt.woodrow) → review+
Updated•9 years ago
|
Attachment #8627038 -
Flags: review?(matt.woodrow) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8627041 [details] [diff] [review] P5. Add MediaSourceSamples logging. Review of attachment 8627041 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/mediasource/TrackBuffersManager.cpp @@ +21,5 @@ > #define MSE_DEBUGV(arg, ...) MOZ_LOG(GetMediaSourceLog(), mozilla::LogLevel::Verbose, ("TrackBuffersManager(%p:%s)::%s: " arg, this, mType.get(), __func__, ##__VA_ARGS__)) > > +PRLogModuleInfo* GetMediaSourceSamplesLog() > +{ > + static PRLogModuleInfo* sLogModule; static PRLogModuleInfo* sLogModule = nullptr; @@ +1244,1 @@ > "kf:%d)", Hanging indent is off here now.
Attachment #8627041 -
Flags: review?(cpearce) → review+
Updated•9 years ago
|
Attachment #8627042 -
Flags: review?(cpearce) → review+
Comment on attachment 8627040 [details] [diff] [review] P4. Process an entire media segment at a time rather than frame by frame. Review of attachment 8627040 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed (unless irrelevant or in error). ::: dom/media/mediasource/TrackBuffersManager.cpp @@ -554,5 @@ > - i++; > - } > - } > - // 4. Remove decoding dependencies of the coded frames removed in the previous step: > - // Remove all coded frames between the coded frames removed in the previous step and the next random access point after those removed frames. You should probably keep this spec-related comment. Or if more relevant: Move it where appropriate, or explain why you don't need to explicitly do it. @@ +554,5 @@ > + } > + if (HasAudio()) { > + MSE_DEBUG("after audio ranges=%s", > + DumpTimeRanges(mAudioTracks.mBufferedRanges).get()); > + } Shouldn't you put these two if blocks behind an #ifdef DEBUG (or similar), to avoid unneeded tests in release mode? @@ +1099,5 @@ > MOZ_ASSERT(OnTaskQueue()); > MSE_DEBUG("mAbort:%d", static_cast<bool>(mAbort)); > > // 1. For each coded frame in the media segment run the following steps: > + // Coded Frame Processing steps 1.1 to to 1.21. 'to to' -> 'to' @@ +1260,5 @@ > + // Let presentation timestamp be a double precision floating point representation of the coded frame's presentation timestamp in seconds. > + // Let decode timestamp be a double precision floating point representation of the coded frame's decode timestamp in seconds. > + > + // 2. Let frame duration be a double precision floating point representation of the coded frame's duration in seconds. > + // 3. If mode equals "sequence" and group start timestamp is set, then run the following steps: Step 3 have moved above, so it should probably removed from here; Or add a note about its move. @@ +1261,5 @@ > + // Let decode timestamp be a double precision floating point representation of the coded frame's decode timestamp in seconds. > + > + // 2. Let frame duration be a double precision floating point representation of the coded frame's duration in seconds. > + // 3. If mode equals "sequence" and group start timestamp is set, then run the following steps: > + // 4. If timestampOffset is not 0, then run the following steps: This documentation is confusing! If you want to keep spec quotes, it might be good to have them all, and/or to explain what's done differently in this implementation. @@ +1264,5 @@ > + // 3. If mode equals "sequence" and group start timestamp is set, then run the following steps: > + // 4. If timestampOffset is not 0, then run the following steps: > + > + TimeInterval sampleInterval = > + mParent->mGenerateTimestamp 'mGenerateTimestamp' -> 'mGenerateTimestamps' (plural) to match specs. @@ +1352,5 @@ > + sample->mTimecode = decodeTimestamp.ToMicroseconds(); > + sample->mTrackInfo = trackBuffer.mLastInfo; > + samples.AppendElement(sample); > + > + // Steps 11,12,13,14, 15 and 16 will be done in one block in InsertBlockFrames. 'InsertBlockFrames' -> 'InsertFrames' @@ +1545,5 @@ > + MSE_DEBUG("Next sample to be played got evicted"); > + aTrackData.mNextGetSampleIndex.reset(); > + } else if (aTrackData.mNextGetSampleIndex.ref() > lastRemovedIndex) { > + aTrackData.mNextGetSampleIndex.ref() -= > + lastRemovedIndex - firstRemovedIndex.ref() + 1; Please indent this calculation, so it doesn't look like a standalone statement.
Attachment #8627040 -
Flags: review?(gsquelart) → review+
Updated•9 years ago
|
Attachment #8627039 -
Flags: review?(ajones) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jyavenard
https://hg.mozilla.org/integration/mozilla-inbound/rev/d376311b06e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b866f804eac4 https://hg.mozilla.org/integration/mozilla-inbound/rev/ef977473f823 https://hg.mozilla.org/integration/mozilla-inbound/rev/727c3f481dbb https://hg.mozilla.org/integration/mozilla-inbound/rev/ce93a71adfbd https://hg.mozilla.org/integration/mozilla-inbound/rev/08840ce62b9a
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d376311b06e7 https://hg.mozilla.org/mozilla-central/rev/b866f804eac4 https://hg.mozilla.org/mozilla-central/rev/ef977473f823 https://hg.mozilla.org/mozilla-central/rev/727c3f481dbb https://hg.mozilla.org/mozilla-central/rev/ce93a71adfbd https://hg.mozilla.org/mozilla-central/rev/08840ce62b9a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
Target Milestone: mozilla41 → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•