Prefer newer media in a TrackBuffer for a given range

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 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

5 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

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
Work in progress mochitest.  Currently fails, but for the wrong reasons.  Logging shows ReadMetadata failing; will investigate more.
Assignee

Comment 2

5 years ago
Posted patch mochitest (obsolete) — Splinter Review
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

5 years ago
Attachment #8484800 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8484805 - Attachment description: mochitest. → mochitest
Assignee

Comment 3

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

Comment 4

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

Comment 5

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

Updated

5 years ago
Attachment #8484805 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Assignee

Comment 7

5 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 9

5 years ago
Attachment #8485568 - Attachment is obsolete: true
Attachment #8485568 - Flags: review?(cajbir.bugzilla)
Attachment #8485591 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 10

5 years ago
Attachment #8485569 - Attachment is obsolete: true
Attachment #8485569 - Flags: review?(cajbir.bugzilla)
Attachment #8485592 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 11

5 years ago
Attachment #8485570 - Attachment is obsolete: true
Attachment #8485570 - Flags: review?(cajbir.bugzilla)
Attachment #8485593 - Flags: review?(cajbir.bugzilla)
Assignee

Comment 12

5 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

5 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

5 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

5 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

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

Comment 16

5 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

5 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

5 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

5 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

5 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.
Depends on: 1064826
Depends on: 1065133
You need to log in before you can comment on or make changes to this bug.