Optimise TrackBuffersManager speed

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 8627037 [details] [diff] [review]
P1. Add IntervalSet operator-=.
Attachment #8627037 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

4 years ago
Created attachment 8627038 [details] [diff] [review]
P2. Add TimeUnit operator/= (int).
Attachment #8627038 - Flags: review?(matt.woodrow)
(Assignee)

Comment 3

4 years ago
Created attachment 8627039 [details] [diff] [review]
P3. Returns error for invalid web mimetype.

Invalid error rather than not supported.
Attachment #8627039 - Flags: review?(ajones)
(Assignee)

Comment 4

4 years ago
Created attachment 8627040 [details] [diff] [review]
P4. Process an entire media segment at a time rather than frame by frame.

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

4 years ago
Created attachment 8627041 [details] [diff] [review]
P5. Add MediaSourceSamples logging.

This allows for MediaSource log to be usable as displaying log samples is
extremely verbose.
Attachment #8627041 - Flags: review?(cpearce)
(Assignee)

Comment 6

4 years ago
Created attachment 8627042 [details] [diff] [review]
P6. Use MediaSourceSamples logging in ContainerParser.
Attachment #8627042 - Flags: review?(cpearce)
Attachment #8627037 - Flags: review?(matt.woodrow) → review+
Attachment #8627038 - Flags: review?(matt.woodrow) → review+
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+
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+
Attachment #8627039 - Flags: review?(ajones) → review+
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Target Milestone: mozilla41 → mozilla42
You need to log in before you can comment on or make changes to this bug.