Closed
Bug 1062664
Opened 10 years ago
Closed 10 years ago
Prefer newer media in a TrackBuffer for a given range
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: kinetik, Assigned: kinetik)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 6 obsolete files)
130.95 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
7.53 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
6.15 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
4.05 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
MediaSourceReader::Switch{Audio,Video}Reader iterates each TrackBuffer's decoder array in order, selecting the first that matches. This is incorrect behaviour -- later decoders providing a media range should be considered in preference to earlier ones.
Assignee | ||
Comment 1•10 years ago
|
||
Work in progress mochitest. Currently fails, but for the wrong reasons. Logging shows ReadMetadata failing; will investigate more.
Assignee | ||
Comment 2•10 years ago
|
||
With fetchWithXHR reporting the 404, and the URL for seek_lowres.webm fixed, this does work... by which I mean fail in the expected way.
Assignee | ||
Updated•10 years ago
|
Attachment #8484800 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8484805 -
Attachment description: mochitest. → mochitest
Assignee | ||
Comment 3•10 years ago
|
||
Fails without remaining patches, passes with then.
Attachment #8485568 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 4•10 years ago
|
||
Intended to be a pure refactoring with no logic/observable behaviour changes.
Attachment #8485569 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 5•10 years ago
|
||
...and the refactoring makes the final fix fairly trivial.
Attachment #8485570 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8484805 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Mochitest is green in debug builds locally, but orange on Try and sometimes orange with a local opt build. Final frame size change seems to be getting lost sometimes... investigating.
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8485568 -
Attachment is obsolete: true
Attachment #8485568 -
Flags: review?(cajbir.bugzilla)
Attachment #8485591 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8485569 -
Attachment is obsolete: true
Attachment #8485569 -
Flags: review?(cajbir.bugzilla)
Attachment #8485592 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8485570 -
Attachment is obsolete: true
Attachment #8485570 -
Flags: review?(cajbir.bugzilla)
Attachment #8485593 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 12•10 years ago
|
||
Remove the ok check from MSR::Seek now that Switch{Audio,Video}Reader
may return false; return to relying on the ContainsTime assert.
Attachment #8485594 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 13•10 years ago
|
||
Any On{Audio,Video}Decoded callbacks triggered before we request a new
sample from a reader after seeking should not be used to compute the
last sample time because they originate from the reader we're switching
away from and cause us to switch back to it after seeking.
Attachment #8485595 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 14•10 years ago
|
||
Any On{Audio,Video}Decoded callbacks triggered before we request a new
sample from a reader after seeking should not be used to compute the
last sample time because they originate from the reader we're switching
away from and cause us to switch back to it after seeking.
Same as last patch, except fixes some logging messages I forgot to update.
Attachment #8485595 -
Attachment is obsolete: true
Attachment #8485595 -
Flags: review?(cajbir.bugzilla)
Attachment #8485614 -
Flags: review?(cajbir.bugzilla)
Comment 15•10 years ago
|
||
Comment on attachment 8485591 [details] [diff] [review]
p1: Frame selection test.
Review of attachment 8485591 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with either the changes mentioned in the test, or a bug raised to move all tests over to dealing with the fact that appendBuffer, endOfStream and the like need to deal with an asynchronous update algorithm so we have a record that the change needs to be made. Disregard if such a bug already exists.
::: content/media/mediasource/test/test_FrameSelection.html
@@ +41,5 @@
> + fetchWithXHR("seek_lowres.webm", function (arrayBuffer) {
> + // Append initialization segment.
> + sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 438));
> + // Append media segment covering range [2, 4].
> + sb.appendBuffer(new Uint8Array(arrayBuffer, 51003));
Needs to be done in 'update' or 'updateend' event.
@@ +42,5 @@
> + // Append initialization segment.
> + sb.appendBuffer(new Uint8Array(arrayBuffer, 0, 438));
> + // Append media segment covering range [2, 4].
> + sb.appendBuffer(new Uint8Array(arrayBuffer, 51003));
> + ms.endOfStream();
Needs to be done when all 'appendBuffer' calls are completed.
Updated•10 years ago
|
Attachment #8485591 -
Flags: review?(cajbir.bugzilla) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8485592 [details] [diff] [review]
p2: Refactor reader switching.
Review of attachment 8485592 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/mediasource/MediaSourceReader.h
@@ +104,5 @@
> private:
> bool SwitchAudioReader(double aTarget);
> bool SwitchVideoReader(double aTarget);
>
> + already_AddRefed<MediaDecoderReader> SelectReader(double aTarget,
Add a comment explaining what this function does. How does 'aCanUser' callback affect what it does and what the return value means. Explain what the passed array of 'aTrackDecoders' is for and/or what it's expected to be. Include what monitor needs to be held by the caller.
@@ +108,5 @@
> + already_AddRefed<MediaDecoderReader> SelectReader(double aTarget,
> + bool (MediaSourceReader::*aCanUseReader)(MediaDecoderReader*),
> + const nsTArray<nsRefPtr<SourceBufferDecoder>>& aTrackDecoders);
> +
> + bool CanUseAudioReader(MediaDecoderReader* aNewReader);
Add a comment explaining what these functions do. What does it mean to be able to 'use' or 'not use' the reader. Perhaps 'CanSelect....' is a better name as they're used to decide if a reader can be selected or not.
Attachment #8485592 -
Flags: review?(cajbir.bugzilla) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8485593 [details] [diff] [review]
p3: Select newest decoder for a given TrackBuffer.
Review of attachment 8485593 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/mediasource/MediaSourceReader.cpp
@@ +274,5 @@
> const nsTArray<nsRefPtr<SourceBufferDecoder>>& aTrackDecoders)
> {
> mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
>
> + for (int32_t i = aTrackDecoders.Length() - 1; i >= 0; --i) {
Add a comment explaining why the iteration is being done in the reverse order.
Attachment #8485593 -
Flags: review?(cajbir.bugzilla) → review+
Comment 18•10 years ago
|
||
Comment on attachment 8485594 [details] [diff] [review]
p4: Make Switch{Audio,Video}Reader return true only on an actual reader change (i.e. selecting the current reader is not a switch).
Review of attachment 8485594 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/mediasource/MediaSourceReader.cpp
@@ +145,5 @@
> mTimeThreshold = aTimeThreshold;
> mDropAudioBeforeThreshold = true;
> mDropVideoBeforeThreshold = true;
> }
> + mVideoIsSeeking = false;
What is this change for? Is it also needed for audio? Add a comment explaining.
Attachment #8485594 -
Flags: review?(cajbir.bugzilla) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8485614 [details] [diff] [review]
p5: Don't update MediaSourceReader::mLast{Audio,Video}Time when seeking.
Review of attachment 8485614 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/mediasource/MediaSourceReader.cpp
@@ +83,5 @@
> MSE_DEBUG("MediaSourceReader(%p)::RequestAudioData called with no audio reader", this);
> GetCallback()->OnDecodeError();
> return;
> }
> + mAudioIsSeeking = false;
I commented about the absence of this in a earlier patch so you can disregard that.
Attachment #8485614 -
Flags: review?(cajbir.bugzilla) → review+
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to cajbir (:cajbir) from comment #15)
> Comment on attachment 8485591 [details] [diff] [review]
> p1: Frame selection test.
>
> Review of attachment 8485591 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ with either the changes mentioned in the test, or a bug raised to move
> all tests over to dealing with the fact that appendBuffer, endOfStream and
> the like need to deal with an asynchronous update algorithm so we have a
> record that the change needs to be made. Disregard if such a bug already
> exists.
I'll change (all of) the tests in a separate bug, along with a simple patch to make the update run partially async so the behaviour will match the spec.
(In reply to cajbir (:cajbir) from comment #19)
> I commented about the absence of this in a earlier patch so you can
> disregard that.
They should've been in the same patch, sorry. Must've squashed the wrong patches there. I'll fix when pushing.
Assignee | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d742663d733d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf7d475651b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8eaf4b01762
https://hg.mozilla.org/integration/mozilla-inbound/rev/0411677efc84
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6dcab0e9e83
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d742663d733d
https://hg.mozilla.org/mozilla-central/rev/5cf7d475651b
https://hg.mozilla.org/mozilla-central/rev/b8eaf4b01762
https://hg.mozilla.org/mozilla-central/rev/0411677efc84
https://hg.mozilla.org/mozilla-central/rev/f6dcab0e9e83
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•