Closed Bug 1096089 Opened 10 years ago Closed 9 years ago

Implement MSE coded frame removal algorithm

Categories

(Core :: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox35 --- unaffected
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- affected

People

(Reporter: cajbir, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 8 obsolete files)

30.16 KB, patch
Details | Diff | Splinter Review
8.61 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.17 KB, patch
Details | Diff | Splinter Review
3.38 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
2.84 KB, patch
cajbir
: review+
bholley
: review+
Details | Diff | Splinter Review
The coded frame removal algorithm is defined in the MSE spec:

https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-coded-frame-removal

This is currently a TODO in the SourceBuffer implementation and needs to be implemented.
Blocks: MSE
Blocks: 881514
Blocks: 1116645, 1116647
No longer blocks: 881514
Blocks: 1118126
Initial incomplete implementation. Works with youtube, resolution change are now instantaneous
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
I'm moving this to P1 as it makes YouTube change of resolution non-working.

When changing resolution, YouTube calls remove(0, duration) on the entire current source buffer. As our remove does nothing, YouTube only adds new resolution data after the end of the current buffered range.

It also causes seek backward to return to the old low resolution.

In my case, YouTube starts playing most video in 144p, which buffers very quickly and if you change resolution after say 10s to 1080p, it has sufficiently buffered close to the entire video (here a 3' video clip).
So we only have 1080p video for the remaining couple of seconds.
Priority: P2 → P1
Quick partial range removal implementation. Data is only ever completely evicted if we are clearing the entire buffered range ; which is what YouTube does. In other cases, we only trim the data when end time is prior the current playback position.
Attachment #8552912 - Flags: review?(matt.woodrow)
Attachment #8552822 - Attachment is obsolete: true
Comment on attachment 8552912 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

cajbit if you could review the eviction itself. I've pretty much copy/paste the existing eviction for when we're evicting before current playback position.

I have proper eviction as per spec on the way.
Attachment #8552912 - Flags: review?(cajbir.bugzilla)
My earlier patch was guaranteed to work only with the patches I have queued from bug 1118589. Must run synchronously if appendBuffer is synchronous.
Attachment #8552912 - Attachment is obsolete: true
Attachment #8552912 - Flags: review?(matt.woodrow)
Attachment #8552912 - Flags: review?(cajbir.bugzilla)
Attachment #8552999 - Flags: review?(matt.woodrow)
Attachment #8552999 - Flags: review?(cajbir.bugzilla)
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good! Not finishing the review yet, since you said there were still bugs to be fixed.

::: dom/media/mediasource/SourceBuffer.cpp
@@ +259,5 @@
> +    // abort was called in between.
> +    return;
> +  }
> +  if (mTrackBuffer) {
> +    int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);

Shouldn't we just return without doing anything if aStart is infinite?

::: dom/media/mediasource/SourceBufferDecoder.h
@@ +121,5 @@
>    // cached data. Returns -1 if no such value is computable.
>    int64_t ConvertToByteOffset(double aTime);
>  
> +  // all durations are in usecs
> +  void Trim(int64_t aDuration);

Can you add some comments explaining that this is basically a hack to workaround the fact that we can't accurately remove coded frames at the moment. Also that we don't really remove any data, just 'hide' it and prevent playback beyond the trim point.

::: dom/media/mediasource/TrackBuffer.cpp
@@ +728,5 @@
> +  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
> +
> +  nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> +  double end = Buffered(buffered) * USECS_PER_S;
> +  double start = buffered->GetStartTime() * USECS_PER_S;

naming of start/end and aStart/aEnd could be improved to make this clearer.

bufferedStart, removeStart maybe?

@@ +733,5 @@
> +
> +  if (aStart >= end ||
> +      (aEnd < end && aEnd >= aPlaybackTime)) {
> +    // TODO. We only handle trimming and removal before current playback position.
> +    return false;

Throw an NS_WARNING in here maybe? That should let us know when we hit this, so we don't spend time debugging failures caused by it.

@@ +762,5 @@
> +          continue;
> +        }
> +        MSE_DEBUG("TrackBuffer(%p):RangeRemoval remove empty decoders=%d", this, i);
> +        RemoveDecoder(decoders[i]);
> +      }

Would be nice to share this code with EvictData
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8552999 [details] [diff] [review]
> MSE: Partially implement Range Removal algorithm
> 
> Review of attachment 8552999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good! Not finishing the review yet, since you said there were
> still bugs to be fixed.

the bugs are triggered by this patch, but I don't believe they are in this patch ...

On the VM where I can reproduce it, I have to change resolution a dozen times before it happens. so pretty minor I think.

I will submit another patch once I identify the problem in MDSM

> 
> ::: dom/media/mediasource/SourceBuffer.cpp
> @@ +259,5 @@
> > +    // abort was called in between.
> > +    return;
> > +  }
> > +  if (mTrackBuffer) {
> > +    int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);
> 
> Shouldn't we just return without doing anything if aStart is infinite?

we could yes...

> Can you add some comments explaining that this is basically a hack to
> workaround the fact that we can't accurately remove coded frames at the
> moment. Also that we don't really remove any data, just 'hide' it and
> prevent playback beyond the trim point.

ok

> Would be nice to share this code with EvictData

ultimately, evict data is supposed to call RangeRemoval ...
Earlier on, all RangeRemoval did was trim, I added cutting from the beginning as an afterthought.

thanks for the review.
Only return END_OF_STREAM when decoding from an ended media source if we're near its end. This fixes audio continuing to play while video is frozen.
Attachment #8553470 - Flags: review?(matt.woodrow)
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------

Trimming results in the Buffered value returning less than the actual data we are holding. This is used in the EvictData call to identify when to evict the data. We need some way of telling EvictData the actual data that is held so it knows when to evict. A GetActualBuffered() type of call it can use. I think this can be done in another bug - either create one for it or point to an existing bug the work can be done under.

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +123,5 @@
>      mAudioPromise.Reject(CANCELED, __func__);
>      return p;
>    }
> +  SwitchReaderResult ret = SwitchAudioReader(mLastAudioTime);
> +  if (ret == READER_ERROR) {

Use 'switch' and 'case' on the valid values of SwitchReaderResult.

@@ +228,1 @@
>    if (SwitchAudioReader(mLastAudioTime, EOS_FUZZ_US)) {

This will succeed on READER_ERROR too - was that intended? The Video version later checks for READER_NEW.

@@ +258,5 @@
>      mVideoPromise.Reject(CANCELED, __func__);
>      return p;
>    }
> +  SwitchReaderResult ret = SwitchVideoReader(mLastVideoTime);
> +  if (ret == READER_ERROR) {

Switch/case please.

::: dom/media/mediasource/SourceBuffer.cpp
@@ +35,5 @@
>  #define MSE_DEBUGV(...)
>  #define MSE_API(...)
>  #endif
>  
> +#define APPENDBUFFER_IS_SYNCHRONOUS 1

Comment why this is needed. Is it a workaround for something? Why would I set it to zero, or undefine it? Is it zero to change the behaviour or undefined changes the behaviour?

@@ +240,2 @@
>  
> +#if APPENDBUFFER_IS_SYNCHRONOUS

I would prefer "#if defined(APPENDBUFFER_IS_SYNCHRONOUS")  and undefined meaning the opposite, rather than setting to 1/0 but no matter which you choose, add a comment somewhere explaining which way it is.

::: dom/media/mediasource/SourceBuffer.h
@@ +110,5 @@
>    double GetBufferedEnd();
>  
>    // Runs the range removal algorithm as defined by the MSE spec.
>    void RangeRemoval(double aStart, double aEnd);
> +  void DoRangeRemoval(double aStart, double aEnd);

Comment explaining difference between this and RangeRemoval.

::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +243,5 @@
>  nsresult
>  SourceBufferDecoder::GetBuffered(dom::TimeRanges* aBuffered)
>  {
> +  nsresult rv = mReader->GetBuffered(aBuffered);
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE macros are asked not to be used in new code in the coding style. Change to:

if (NS_FAILED(rv)) {
  return rv;
}

::: dom/media/mediasource/SourceBufferDecoder.h
@@ +120,5 @@
>    // Given a time convert it into an approximate byte offset from the
>    // cached data. Returns -1 if no such value is computable.
>    int64_t ConvertToByteOffset(double aTime);
>  
> +  // all durations are in usecs

Capital A.

@@ +121,5 @@
>    // cached data. Returns -1 if no such value is computable.
>    int64_t ConvertToByteOffset(double aTime);
>  
> +  // all durations are in usecs
> +  void Trim(int64_t aDuration);

Explain whether this is a TrimFromStart, or a TrimFromEnd.

@@ +127,5 @@
> +  {
> +    return mTrimmedOffset >= 0;
> +  }
> +
> +  void SetRealMediaDuration(int64_t aDuration);

Comment saying what a 'Real Media Duration' is vs a non-real media duration. Say what the units are. Same for return value of GetRealMediaDuration. You have an "all durations are in usecs" further above but I think it's better to have it listed with each function so people don't need to go searching for it.

@@ +148,2 @@
>    int64_t mTimestampOffset;
>    int64_t mMediaDuration;

Explain difference between mMediaDuration and mRealMediaDuration - either here or in the function as mentioned previously.

@@ +148,5 @@
>    int64_t mTimestampOffset;
>    int64_t mMediaDuration;
> +  int64_t mRealMediaDuration;
> +  // in seconds
> +  double mTrimmedOffset;

Explain what exactly this is. Is it trimming before, or after the offset for example.

::: dom/media/mediasource/TrackBuffer.h
@@ +94,5 @@
>  
> +  // Runs MSE range removal algorithm.
> +  // http://w3c.github.io/media-source/#sourcebuffer-coded-frame-removal
> +  // Implementation is only partial, we can only trim a buffer, or remove
> +  // data found prior the existing playback position.

"prior to the"

@@ +96,5 @@
> +  // http://w3c.github.io/media-source/#sourcebuffer-coded-frame-removal
> +  // Implementation is only partial, we can only trim a buffer, or remove
> +  // data found prior the existing playback position.
> +  // Returns true if data was evicted.
> +  bool RangeRemoval(int64_t aPlaybackTime, int64_t aStart, int64_t aEnd);

Mention what units these arguments are in.

@@ +119,5 @@
>    already_AddRefed<SourceBufferDecoder> NewDecoder(int64_t aTimestampOffset /* microseconds */);
>  
>    // Helper for AppendData, ensures NotifyDataArrived is called whenever
>    // data is appended to the current decoder's SourceBufferResource.
> +  bool AppendDataToCurrentResource(const uint8_t* aData, uint32_t aLength, uint32_t aDuration);

Mention what unit aDuration is.
Attachment #8552999 - Flags: review?(cajbir.bugzilla) → review+
Some quick explanations:

(In reply to cajbir (:cajbir) from comment #10)

> Use 'switch' and 'case' on the valid values of SwitchReaderResult.
ok.

> 
> @@ +228,1 @@
> >    if (SwitchAudioReader(mLastAudioTime, EOS_FUZZ_US)) {
> 
> This will succeed on READER_ERROR too - was that intended? The Video version
> later checks for READER_NEW.
> 

I simply keep the existing behaviour for code path my code do not affect. but yes, this should be done too (as I believe what we had was buggy)

> Comment why this is needed. Is it a workaround for something? Why would I
> set it to zero, or undefine it? Is it zero to change the behaviour or
> undefined changes the behaviour?

this is while waiting for bug 1118589 to land...

if appendbuffer is made asynchronous, then range removal must be too.

Thanks for the review..
Comment on attachment 8553470 [details] [diff] [review]
Only return end of stream if we're near the known duration

Review of attachment 8553470 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +940,5 @@
> +bool
> +MediaSourceReader::IsNearEnd(int64_t aTime)
> +{
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  return mEnded && aTime >= (mMediaSourceDuration * USECS_PER_S - EOS_FUZZ_US);

This seems like a large amount of fuzz.
Attachment #8553470 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8552999 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

Review of attachment 8552999 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good! Not finishing the review yet, since you said there were still bugs to be fixed.

::: dom/media/mediasource/SourceBuffer.cpp
@@ +259,5 @@
> +    // abort was called in between.
> +    return;
> +  }
> +  if (mTrackBuffer) {
> +    int64_t start = IsInfinite(aStart) ? INT64_MAX : (int64_t)(aStart * USECS_PER_S);

Shouldn't we just return without doing anything if aStart is infinite?

::: dom/media/mediasource/SourceBufferDecoder.h
@@ +121,5 @@
>    // cached data. Returns -1 if no such value is computable.
>    int64_t ConvertToByteOffset(double aTime);
>  
> +  // all durations are in usecs
> +  void Trim(int64_t aDuration);

Can you add some comments explaining that this is basically a hack to workaround the fact that we can't accurately remove coded frames at the moment. Also that we don't really remove any data, just 'hide' it and prevent playback beyond the trim point.

::: dom/media/mediasource/TrackBuffer.cpp
@@ +728,5 @@
> +  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
> +
> +  nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> +  double end = Buffered(buffered) * USECS_PER_S;
> +  double start = buffered->GetStartTime() * USECS_PER_S;

naming of start/end and aStart/aEnd could be improved to make this clearer.

bufferedStart, removeStart maybe?

@@ +733,5 @@
> +
> +  if (aStart >= end ||
> +      (aEnd < end && aEnd >= aPlaybackTime)) {
> +    // TODO. We only handle trimming and removal before current playback position.
> +    return false;

Throw an NS_WARNING in here maybe? That should let us know when we hit this, so we don't spend time debugging failures caused by it.

@@ +762,5 @@
> +          continue;
> +        }
> +        MSE_DEBUG("TrackBuffer(%p):RangeRemoval remove empty decoders=%d", this, i);
> +        RemoveDecoder(decoders[i]);
> +      }

Would be nice to share this code with EvictData
Attachment #8552999 - Flags: review?(matt.woodrow) → review+
Carrying both r+. Thanks guys. Removed the trimming before the current playback position. It leaves the source buffer resource in unreadable state as both demuxers will choke on invalid data. EvictData must be re-written.
Attachment #8552999 - Attachment is obsolete: true
Rebase following earlier changes (switch vs if)
Attachment #8553470 - Attachment is obsolete: true
Was failing the media-source/mediasource-duration.html webref.
It turns out they set a sourceBuffer with a timestampOffset = 2. Then call appendBuffer with 1s of data. So we have a buffered range of [2,3].
Request*Data was returning WAITING_FOR_DATA while it would have returned the first frame at 2s previously. The MediaDecoderStateMachine is waiting for a first frame to be decoded at time 0 immediately, so I added a test to check if we've already decoded one frame. If not, we default to the old behaviour of using the current decoder rather than return EOF or WAITING.
I don't think either behaviour is right. We still fail the test, just differently.

I wrote a quick test page: http://people.mozilla.org/~jyavenard/tests/mse_mp4/paper-offset.html?eos=1&duration=-1 and checked that our behaviour is the same between Chrome and IE.
This revealed an existing problem in the MDSM: it appears that once we hit WAITING_FOR_DATA we are then unable to seek. Chrome has no issue seeking within the 2-10s existing range.
Attachment #8553544 - Attachment is obsolete: true
Attachment #8553548 - Attachment is obsolete: true
Keywords: leave-open
Depends on: 1116007
Comment on attachment 8553654 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

Review of attachment 8553654 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +249,5 @@
> +  }
> +  if (!WasTrimmed()) {
> +    return NS_OK;
> +  }
> +  nsRefPtr<dom::TimeRanges> tr = new dom::TimeRanges();

Missing #include "mozilla/dom/TimeRanges.h"
Depends on: 1125588
Comment on attachment 8553654 [details] [diff] [review]
MSE: Partially implement Range Removal algorithm

Requesting uplift for the first two patches on this bug, plus the unified build fix.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: YouTube playback can show lower quality video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This is moderately risky, but is MSE-specific. We'd like to try it for testing and see if it addresses issue with Firefox no switching to higher quality video properly.
[String/UUID change made/needed]: None.
Attachment #8553654 - Flags: approval-mozilla-beta?
Attachment #8553654 - Flags: approval-mozilla-aurora?
Re-introduce code that I later removed. That's to evict data before current playback position. This was r+ already
Attachment #8553654 - Flags: approval-mozilla-beta?
Attachment #8553654 - Flags: approval-mozilla-beta+
Attachment #8553654 - Flags: approval-mozilla-aurora?
Attachment #8553654 - Flags: approval-mozilla-aurora+
Depends on: 1125776
Attached patch Update webref tests (obsolete) — Splinter Review
Update webref. Like bug 1125776, we fail due to bug 1065207. We're off by a few milliseconds in the duration reported on webm. And for mp4, I don't know where they get their 6s from (the init segment says it's 15s)
Attachment #8557429 - Flags: review?(cajbir.bugzilla)
Comment on attachment 8557429 [details] [diff] [review]
Update webref tests

I defer to karl on web platform test changes.
Attachment #8557429 - Flags: review?(cajbir.bugzilla) → review?(karlt)
Comment on attachment 8557429 [details] [diff] [review]
Update webref tests

I expect the changes in this bug haven't actually changed the results here, but I think the logic is better.  (These tests don't run on Macs in automation yet.)

So I'd suggest something like "Generalize [Test remove with a start at the duration.] expected result to all platforms using mp4" for a comment.
Attachment #8557429 - Flags: review?(karlt) → review+
Carrying r+
Attachment #8557429 - Attachment is obsolete: true
Assuming we can close this now.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Add removal from beginning of source buffer. That adds support for both type of trimming (from left and from right). Adding support of removing data in the middle of the source buffer with our existing architecture is likely a moot point. This patch is very similar to the one you r+ earlier, except that it doesn't restrict to truncation before the current playback time as the MediaSourceReader will handle the condition properly
Attachment #8567711 - Flags: review?(cajbir.bugzilla)
Attachment #8555227 - Attachment is obsolete: true
Attachment #8567711 - Flags: review?(cajbir.bugzilla) → review+
This bug is about the implementation of Range Removal algorithm. 

The final part about removing data in the middle of a source buffer isn't implemented yet
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Be compliant with spec: end is an unrestricted double. Test for updating attribute first. Update tests
Attachment #8568442 - Flags: review?(cajbir.bugzilla)
https://hg.mozilla.org/mozilla-central/rev/5a745cf431cb
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #8568442 - Flags: review?(cajbir.bugzilla) → review+
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec

Bobby, I need the WebIDL part reviewed by a "DOM peer" and since you appear to not just be doing media anymore :)
Attachment #8568442 - Flags: review?(bobbyholley)
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec

Review of attachment 8568442 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the webidl change.
Attachment #8568442 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8e7a594541

(the regex checking for DOM peer needs to be fixed, it must have a space before r=)
beta uplift for last two patches
Flags: needinfo?(giles)
Comment on attachment 8567711 [details] [diff] [review]
Add trimming support before playback position

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance. Possible stalls playing MSE video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Implements missing MSE functionality. No regression risk outside that feature.
[String/UUID change made/needed]: None.
Flags: needinfo?(giles)
Attachment #8567711 - Flags: approval-mozilla-beta?
Attachment #8567711 - Flags: approval-mozilla-aurora?
Comment on attachment 8568442 [details] [diff] [review]
Make end argument an unrestricted double as per spec

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance, less consistent testing.
[Describe test coverage new/current, TreeHerder]: Landed on m-c. We pass more web platform tests with this patch.
[Risks and why]: Small MSE-specific change, so low.
[String/UUID change made/needed]: None.
Attachment #8568442 - Flags: approval-mozilla-beta?
Attachment #8568442 - Flags: approval-mozilla-aurora?
Attachment #8567711 - Flags: approval-mozilla-beta?
Attachment #8567711 - Flags: approval-mozilla-beta+
Attachment #8567711 - Flags: approval-mozilla-aurora?
Attachment #8567711 - Flags: approval-mozilla-aurora+
Attachment #8568442 - Flags: approval-mozilla-beta?
Attachment #8568442 - Flags: approval-mozilla-beta+
Attachment #8568442 - Flags: approval-mozilla-aurora?
Attachment #8568442 - Flags: approval-mozilla-aurora+
Reset the status for these two new patch, fyi Ryan.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Target Milestone: mozilla38 → ---
Needs rebasing for beta.
Flags: needinfo?(giles)
Flags: needinfo?(giles)
Depends on: 1138260
Please open a follow-up bug for any remaining issues here.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: needinfo?(jyavenard)
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1151375
spawned in 1151375
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.