1062664 - Prefer newer media in a TrackBuffer for a given range

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: mochitest

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

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: p1: Frame selection mochitest.

Fails without remaining patches, passes with then.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: p2: Refactor reader switching.

Intended to be a pure refactoring with no logic/observable behaviour changes.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: p3: Select newest decoder for a given TrackBuffer.

...and the refactoring makes the final fix fairly trivial.

Comment 6

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=242722dd0f11

Comment 7

[Matthew Gregan [:kinetik]]

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.

Comment 8

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=26d387d91924

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: p1: Frame selection test.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: p2: Refactor reader switching.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: p3: Select newest decoder for a given TrackBuffer.

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 14

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 15

[cajbir (:cajbir)]

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.

Comment 16

[cajbir (:cajbir)]

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.

Comment 17

[cajbir (:cajbir)]

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.

Comment 18

[cajbir (:cajbir)]

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.

Comment 19

[cajbir (:cajbir)]

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.

Comment 20

[Matthew Gregan [:kinetik]]

(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.

Comment 21

[Matthew Gregan [:kinetik]]

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

[Carsten Book [:Tomcat]]

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