1171760 - Optimise TrackBuffersManager speed

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Add IntervalSet operator-=.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add TimeUnit operator/= (int).

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P3. Returns error for invalid web mimetype.

Invalid error rather than not supported.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: 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.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P5. Add MediaSourceSamples logging.

This allows for MediaSource log to be usable as displaying log samples is
extremely verbose.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P6. Use MediaSourceSamples logging in ContainerParser.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 9

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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