Prefer newer media in a TrackBuffer for a given range

RESOLVED FIXED in mozilla35

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

(Blocks: 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

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
(Assignee)

Description

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

3 years ago
Created attachment 8484800 [details] [diff] [review]
mochitest

Work in progress mochitest.  Currently fails, but for the wrong reasons.  Logging shows ReadMetadata failing; will investigate more.
(Assignee)

Comment 2

3 years ago
Created attachment 8484805 [details] [diff] [review]
mochitest

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

3 years ago
Attachment #8484800 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8484805 - Attachment description: mochitest. → mochitest
(Assignee)

Comment 3

3 years ago
Created attachment 8485568 [details] [diff] [review]
p1: Frame selection mochitest.

Fails without remaining patches, passes with then.
Attachment #8485568 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 4

3 years ago
Created attachment 8485569 [details] [diff] [review]
p2: Refactor reader switching.

Intended to be a pure refactoring with no logic/observable behaviour changes.
Attachment #8485569 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 5

3 years ago
Created attachment 8485570 [details] [diff] [review]
p3: Select newest decoder for a given TrackBuffer.

...and the refactoring makes the final fix fairly trivial.
Attachment #8485570 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8484805 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=242722dd0f11
(Assignee)

Updated

3 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 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

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=26d387d91924
(Assignee)

Comment 9

3 years ago
Created attachment 8485591 [details] [diff] [review]
p1: Frame selection test.
Attachment #8485568 - Attachment is obsolete: true
Attachment #8485568 - Flags: review?(cajbir.bugzilla)
Attachment #8485591 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 10

3 years ago
Created attachment 8485592 [details] [diff] [review]
p2: Refactor reader switching.
Attachment #8485569 - Attachment is obsolete: true
Attachment #8485569 - Flags: review?(cajbir.bugzilla)
Attachment #8485592 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 11

3 years ago
Created attachment 8485593 [details] [diff] [review]
p3: Select newest decoder for a given TrackBuffer.
Attachment #8485570 - Attachment is obsolete: true
Attachment #8485570 - Flags: review?(cajbir.bugzilla)
Attachment #8485593 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 12

3 years ago
Created 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).

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

3 years ago
Created attachment 8485595 [details] [diff] [review]
p5: Don't update MediaSourceReader::mLast{Audio,Video}Time when seeking.

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

3 years ago
Created attachment 8485614 [details] [diff] [review]
p5: Don't update MediaSourceReader::mLast{Audio,Video}Time when seeking.

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

3 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

3 years ago
Attachment #8485591 - Flags: review?(cajbir.bugzilla) → review+

Comment 16

3 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

3 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

3 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

3 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

3 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

3 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
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1064826
Depends on: 1065133
You need to log in before you can comment on or make changes to this bug.