Closed Bug 1062664 Opened 6 years ago Closed 6 years ago

Prefer newer media in a TrackBuffer for a given range

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

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.
Attached patch mochitest (obsolete) — Splinter Review
Work in progress mochitest.  Currently fails, but for the wrong reasons.  Logging shows ReadMetadata failing; will investigate more.
Attached 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.
Attachment #8484800 - Attachment is obsolete: true
Attachment #8484805 - Attachment description: mochitest. → mochitest
Attached patch p1: Frame selection mochitest. (obsolete) — Splinter Review
Fails without remaining patches, passes with then.
Attachment #8485568 - Flags: review?(cajbir.bugzilla)
Attached patch p2: Refactor reader switching. (obsolete) — Splinter Review
Intended to be a pure refactoring with no logic/observable behaviour changes.
Attachment #8485569 - Flags: review?(cajbir.bugzilla)
...and the refactoring makes the final fix fairly trivial.
Attachment #8485570 - Flags: review?(cajbir.bugzilla)
Attachment #8484805 - Attachment is obsolete: true
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
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.
Attachment #8485568 - Attachment is obsolete: true
Attachment #8485568 - Flags: review?(cajbir.bugzilla)
Attachment #8485591 - Flags: review?(cajbir.bugzilla)
Attachment #8485569 - Attachment is obsolete: true
Attachment #8485569 - Flags: review?(cajbir.bugzilla)
Attachment #8485592 - Flags: review?(cajbir.bugzilla)
Attachment #8485570 - Attachment is obsolete: true
Attachment #8485570 - Flags: review?(cajbir.bugzilla)
Attachment #8485593 - Flags: review?(cajbir.bugzilla)
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)
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)
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 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.
Attachment #8485591 - Flags: review?(cajbir.bugzilla) → review+
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 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 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 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+
(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.