1287381 - Ensure SetVideoDecodeThreshold() is between PDM->Flush() and PDM->Input()

Status: NEW

(Core :: Audio/Video: Playback, defect, P3)

Description

[Alfredo Yang (:alfredo)]

This is from https://bugzilla.mozilla.org/show_bug.cgi?id=1286441#c9.

There are two conditions needs to call SetVideoDecodeThreshold().
1. After seek operation. SetVideoDecodeThreshold() is called in OnVideoSeekCompleted()

2. stream id changes which causes decoder reinitialized. SetVideoDecodeThreshold() is called in HandleDemuxedSamples()->EnsureDecoderInitialized() promise completed.

We don't need another SetVideoDecodeThreshold() in InternalSeek().

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().

Review commit: https://reviewboard.mozilla.org/r/64856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64856/

Comment 2

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang (:alfredo) from comment #0)
> This is from https://bugzilla.mozilla.org/show_bug.cgi?id=1286441#c9.
> 
> There are two conditions needs to call SetVideoDecodeThreshold().
> 1. After seek operation. SetVideoDecodeThreshold() is called in
> OnVideoSeekCompleted()
> 
> 2. stream id changes which causes decoder reinitialized.
> SetVideoDecodeThreshold() is called in
> HandleDemuxedSamples()->EnsureDecoderInitialized() promise completed.
> 
> We don't need another SetVideoDecodeThreshold() in InternalSeek().

There is one more conditions, one that involve InternalSeek but not change of stream Id: when a gap is encountered and we need to output all the frames up to the gap. 

In this case, you do want to have the threshold set. 
right prior performing the internal seek, the decoder is flushed which would reset the threshold in the decoder.
So are you sure the one in InternalSeek is not necessary? Seems to me that it's important

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771897 [details]
Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().

https://reviewboard.mozilla.org/r/64856/#review62176

::: dom/media/MediaFormatReader.cpp:1049
(Diff revision 1)
>    MOZ_ASSERT(OnTaskQueue());
>    LOG("%s internal seek to %f",
>        TrackTypeToStr(aTrack), aTarget.Time().ToSeconds());
>  
>    auto& decoder = GetDecoderData(aTrack);
>    decoder.Flush();

decoder.flush will call MediaDataDecoder::Flush() which for the mac decoder (as an example seeing it's the only one implementing seek threshold) will do:

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/media/platforms/apple/AppleVDADecoder.cpp#153

mSeekTargetThreshold.reset();

::: dom/media/MediaFormatReader.cpp
(Diff revision 1)
>                        auto& decoder = self->GetDecoderData(aTrack);
>                        decoder.mSeekRequest.Complete();
>                        MOZ_ASSERT(decoder.mTimeThreshold,
>                                   "Seek promise must be disconnected when timethreshold is reset");
>                        decoder.mTimeThreshold.ref().mHasSeeked = true;
> -                      self->SetVideoDecodeThreshold();

So here we have the decoder that has been flushed, the seeking threshold has been cleared and we start decoding.

As a result, the seeking threshold optimisation will not be active if the decoder currently exists. Which will only work in the case where we reach InternalSeek when the streamID changed (as the decoder has been shutdown).

But in the case where it's called via there: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#882
or:
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1220

then you'll not use the seek threshold when you should have.

Comment 4

[Alfredo Yang (:alfredo)]

Comment on attachment 8771897 [details]
Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64856/diff/1-2/

Comment 5

[Alfredo Yang (:alfredo)]

Add SetVideoDecodeThreshold() back to InternalSeek() and call it immediately after flush().

Try run is good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2628bfe9a6b7

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8771897 [details]
Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().

https://reviewboard.mozilla.org/r/64856/#review62190

::: dom/media/MediaFormatReader.cpp
(Diff revision 2)
>                        auto& decoder = self->GetDecoderData(aTrack);
>                        decoder.mSeekRequest.Complete();
>                        MOZ_ASSERT(decoder.mTimeThreshold,
>                                   "Seek promise must be disconnected when timethreshold is reset");
>                        decoder.mTimeThreshold.ref().mHasSeeked = true;
> -                      self->SetVideoDecodeThreshold();

What's the point of moving SetVideoDecodeThreshold prior the seek completing?

what difference does that make?

Comment 7

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8771897 [details]
> Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().
> 
> https://reviewboard.mozilla.org/r/64856/#review62190
> 
> ::: dom/media/MediaFormatReader.cpp
> (Diff revision 2)
> >                        auto& decoder = self->GetDecoderData(aTrack);
> >                        decoder.mSeekRequest.Complete();
> >                        MOZ_ASSERT(decoder.mTimeThreshold,
> >                                   "Seek promise must be disconnected when timethreshold is reset");
> >                        decoder.mTimeThreshold.ref().mHasSeeked = true;
> > -                      self->SetVideoDecodeThreshold();
> 
> What's the point of moving SetVideoDecodeThreshold prior the seek completing?
> 
> what difference does that make?

From log in bug 1286441. There are multiple InternalSeek() calls. The video data slips to decoder before the final InteralSeek() promise is completed. So I move SetVideoDecodeThreshold() immediately after Flush() to ensure there is no video input before SetVideoDecodeThreshold().


1533 19:14:08     INFO -  [MediaPlayback #3]: D/MediaFormatReader MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333

1579 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333

1653 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to 1363333

1655 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to 1363333

1639 19:14:09     INFO -  [MediaPlayback #1]: D/MediaFormatReader MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333

2183 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to decoder

2211 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to decoder

2213 19:14:09     INFO -  [MediaPlayback #4]: D/MediaFormatReader MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to 1363333

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang (:alfredo) from comment #7)
> (In reply to Jean-Yves Avenard [:jya] from comment #6)
> > Comment on attachment 8771897 [details]
> > Bug 1287381: call SetVideoDecodeThreshold() immediately after Flush().
> > 
> > https://reviewboard.mozilla.org/r/64856/#review62190
> > 
> > ::: dom/media/MediaFormatReader.cpp
> > (Diff revision 2)
> > >                        auto& decoder = self->GetDecoderData(aTrack);
> > >                        decoder.mSeekRequest.Complete();
> > >                        MOZ_ASSERT(decoder.mTimeThreshold,
> > >                                   "Seek promise must be disconnected when timethreshold is reset");
> > >                        decoder.mTimeThreshold.ref().mHasSeeked = true;
> > > -                      self->SetVideoDecodeThreshold();
> > 
> > What's the point of moving SetVideoDecodeThreshold prior the seek completing?
> > 
> > what difference does that make?
> 
> From log in bug 1286441. There are multiple InternalSeek() calls. The video
> data slips to decoder before the final InteralSeek() promise is completed.
> So I move SetVideoDecodeThreshold() immediately after Flush() to ensure
> there is no video input before SetVideoDecodeThreshold().
> 
> 
> 1533 19:14:08     INFO -  [MediaPlayback #3]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333
> 
> 1579 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333
> 
> 1653 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to
> 1363333
> 
> 1655 19:14:09     INFO -  [MediaPlayback #2]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to
> 1363333
> 
> 1639 19:14:09     INFO -  [MediaPlayback #1]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333
> 
> 2183 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to
> decoder
> 
> 2211 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader
> MediaFormatReader(1d1e2000)::HandleDemuxedSamples: Giving Video input to
> decoder
> 
> 2213 19:14:09     INFO -  [MediaPlayback #4]: D/MediaFormatReader
> MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to
> 1363333

When the decoder is flushed, there can no longer be any decoding happening. https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.h#373

So you can't have a call to Input while an internal is pending.

Your logs there do not indicate that input was called prior SetVideoDecodeThreshold, the line "Giving Video input to decoder" is displayed as soon as HandleDemuxedSamples is called, and prior MediaDataDecoder::Input is called.

So basically, your changes here serve no purpose: after a flush there can't be any video input happening, it returns early right after InternalSeek
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1009

Comment 9

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> 
> When the decoder is flushed, there can no longer be any decoding happening.
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.
> h#373

What if there is second InternalSeek() before the first InternalSeek() is completed?
The first InternalSeek() promise sets mDecodingRequested to true before the second one is completed. And then video data slips to decoder.

> 
> So you can't have a call to Input while an internal is pending.
> 
> Your logs there do not indicate that input was called prior
> SetVideoDecodeThreshold, the line "Giving Video input to decoder" is
> displayed as soon as HandleDemuxedSamples is called, and prior
> MediaDataDecoder::Input is called. 

The mNumSamplesInput is increased between InternalSeek() and SetVideoDecodeThreshold() called by InternalSeek() promise completed.
Unless it is demux only case, but I think it is not. So it must be video inputs to decoder. 

1639: 19:14:09     INFO -  [MediaPlayback #1]: D/MediaFormatReader MediaFormatReader(1d1e2000)::InternalSeek: Video internal seek to 1.363333

1952: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:11 out:0 qs=11 pending:0 waiting:0 ahead:0 sid:27
1965: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:11 out:0 qs=11 pending:0 waiting:0 ahead:0 sid:27
1978: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:12 out:0 qs=12 pending:0 waiting:0 ahead:0 sid:27
1990: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:12 out:0 qs=12 pending:0 waiting:0 ahead:0 sid:27
2004: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:13 out:0 qs=13 pending:0 waiting:0 ahead:0 sid:27
2016: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:13 out:0 qs=13 pending:0 waiting:0 ahead:0 sid:27
2030: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:14 out:0 qs=14 pending:0 waiting:0 ahead:0 sid:27
2043: 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:14 out:0 qs=14 pending:0 waiting:0 ahead:0 sid:27
2056: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:15 out:0 qs=15 pending:0 waiting:0 ahead:0 sid:27
2068: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:15 out:0 qs=15 pending:0 waiting:0 ahead:0 sid:27
2078: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:16 out:0 qs=16 pending:0 waiting:0 ahead:0 sid:27
2090: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:16 out:0 qs=16 pending:0 waiting:0 ahead:0 sid:27
2101: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:17 out:0 qs=17 pending:0 waiting:0 ahead:0 sid:27
2113: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:17 out:0 qs=17 pending:0 waiting:0 ahead:0 sid:27
2127: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:18 out:0 qs=18 pending:0 waiting:0 ahead:0 sid:27
2139: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:18 out:0 qs=18 pending:0 waiting:0 ahead:0 sid:27
2153: 19:14:09     INFO -  [MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:19 out:0 qs=19 pending:0 waiting:0 ahead:0 sid:27
2168: 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:19 out:0 qs=19 pending:0 waiting:0 ahead:0 sid:27
2176: 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:20 out:0 qs=20 pending:0 waiting:0 ahead:0 sid:27
2181: 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:20 out:0 qs=20 pending:0 waiting:0 ahead:0 sid:27
2196: 19:14:09     INFO -  [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:21 out:0 qs=21 pending:0 waiting:0 ahead:0 sid:27
2210: 19:14:09     INFO -  [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(1d1e2000)::Update: Update(Video) ni=1 no=1 ie=1, in:21 out:0 qs=21 pending:0 waiting:0 ahead:0 sid:27

2213: 19:14:09     INFO -  [MediaPlayback #4]: D/MediaFormatReader MediaFormatReader(1d1e2000)::SetVideoDecodeThreshold: Set seek threshold to 1363333

Comment 10

[Jean-Yves Avenard [:jya]]

You can't have a second InternalSeek happening prior the other being finished. 

You can't have any data being fed to the decoder prior the seek operation in InternalSeek having completed. 

if SetSeekThreshold is called after an Input then it's a bug, but so far not of the changes you've suggested have had anything to do with the situation you describe.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.