Closed Bug 1309516 Opened 8 years ago Closed 7 years ago

Make the MFR returns the start time without decoding the first frame.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Depends on 1 open bug, Regressed 2 open bugs)

Details

Attachments

(10 files, 3 obsolete files)

58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Spawned from discussion starting at bug 1299074 comment 24.

This way, we can remove the DecodeFirstFrame state and make the NON-MSE cases seekable before decoding any frames.
Assignee: nobody → kaku
Depends on: 1309517
:jya,

Once we have the MFR return start time by demuxing only, we can then simplify the MediaDecoderReaderWarpper and MDSM by removing the logic of "decoding first frame to get the start time". However, this we will break legacy readers, do you have any concern?
Flags: needinfo?(jyavenard)
There are 2 issues here:
1. retrieve the start time without decoding first frames.
2. remove the DecodingFirstFrameState state.

I think we still want to keep DecodingFirstFrameState which deal with the 'loadeddata' event. If we remove this state, we will need to have DecodingState deal with the event. I would like to keep each state simple and small.

So the benefit of this bug is to simplify the code of DecodingFirstFrameState, allowing it to seek immediately without storing a queued seek job.
What remaining MediaDecoderReader do we have left?
OMXReader -> about to be removed
AndroidMediaReader -> Is this one still used? If it's an audio only player, then it's okay the first frame will always be 0
DirectShowReader -> This is MP3 only reader, first frame is always 0.
OggReader -> No longer used, code can be removed.
WaveReader -> No longer used, code can be removed

So other than AndroidMediaReader which I don't know about, it's fine..
Flags: needinfo?(jyavenard)
Here are the WIP patches, not pass the try server yet. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=929e24e0781d
Attachment #8803304 - Attachment is obsolete: true
Attachment #8803307 - Attachment is obsolete: true
Attachment #8803310 - Attachment is obsolete: true
Comment on attachment 8805090 [details]
Bug 1309516 part 1 - retrieve start time before resolving the metadata promise;

https://reviewboard.mozilla.org/r/88898/#review88074

Sorry that I did not noticed that there is still a "(version 2)" in the patch comment before publishing this review request, I will remove it at the next phase.
Attachment #8805090 - Flags: review?(kaku)
Attachment #8805090 - Flags: review?(kaku)
Comment on attachment 8805090 [details]
Bug 1309516 part 1 - retrieve start time before resolving the metadata promise;

https://reviewboard.mozilla.org/r/88898/#review88224

::: dom/media/MediaFormatReader.cpp:371
(Diff revision 1)
> +    mVideo.mFirstDemuxedSampleTime.emplace(TimeUnit::FromMicroseconds(0));
> +  } else {
> +    if (HasAudio()) {
> +      RequestDemuxSamples(TrackInfo::kAudioTrack);
> +    } else {
> +      mAudio.mFirstDemuxedSampleTime.emplace(TimeUnit::FromInfinity());

mFirstDemuxedSampleTime is a Maybe, so I don't see why it should be set with a dummy value to be used in calculation later.

::: dom/media/MediaFormatReader.cpp:389
(Diff revision 1)
> +void
> +MediaFormatReader::MaybeResolveMetadataPromise()
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +
> +  if (mAudio.mFirstDemuxedSampleTime.isNothing() ||

if (HasAudio() && mAudio.mFirstDemuxedSampleTime.isNothing() etc...

so that you don't need to store infinity above

::: dom/media/MediaFormatReader.cpp:394
(Diff revision 1)
> +  if (mAudio.mFirstDemuxedSampleTime.isNothing() ||
> +      mVideo.mFirstDemuxedSampleTime.isNothing()) {
> +    return;
> +  }
> +
> +  TimeUnit startTime = std::min(mAudio.mFirstDemuxedSampleTime.ref(),

refOr(TimeUnit::FromInfinity())

::: dom/media/MediaFormatReader.cpp:396
(Diff revision 1)
> +    return;
> +  }
> +
> +  TimeUnit startTime = std::min(mAudio.mFirstDemuxedSampleTime.ref(),
> +                                mVideo.mFirstDemuxedSampleTime.ref());
> +  if (startTime.IsInfinite()) {

how could this happen?

and may as well use mStartTime here, which is default constructed to TimeUnit() that is 0.

in any case, doing:
if (!startTime.IsInfinite) { mInfo.mStartTime = startTime } is more efficient

::: dom/media/MediaFormatReader.cpp:403
(Diff revision 1)
> +  }
> +
> +  mInfo.mStartTime = startTime;
>    RefPtr<MetadataHolder> metadata = new MetadataHolder();
>    metadata->mInfo = mInfo;
> -  metadata->mTags = tags->Count() ? tags.release() : nullptr;
> +  metadata->mTags = mTags->Count() ? mTags.release() : nullptr;

Why not use Move here ?

::: dom/media/MediaFormatReader.cpp:657
(Diff revision 1)
> +                  self->OnFirstDemuxCompleted(TrackInfo::kVideoTrack, aSamples);
> +                },
> +                [self] (const MediaResult& aError) {
> +                  self->OnFirstDemuxFailed(TrackInfo::kVideoTrack, aError);
> +                })
> +         ->CompletionPromise();

I'd prefer this be on the same line as }) above

::: dom/media/MediaInfo.h:508
(Diff revision 1)
>    bool mMediaSeekableOnlyInBufferedRanges = false;
>  
>    EncryptionInfo mCrypto;
> +
> +  // The minimum of start times of audio and video tracks.
> +  // Used to map the zero time on the media timeline to the first frame.

s/Used/Use

but you're not using mStartTime anywhere, and it's already in mInfo so you can drop this unused member.
Attachment #8805090 - Flags: review?(jyavenard) → review+
Comment on attachment 8803305 [details]
Bug 1309516 part 2 - replace MediaFormatReader::DemuxStartTime() with MediaInfo::mStartTime;

https://reviewboard.mozilla.org/r/87614/#review88232
Attachment #8803305 - Flags: review?(jyavenard) → review+
Comment on attachment 8803306 [details]
Bug 1309516 part 3 - make MediaDecoderReaderWrapper keeps the start time returned from reader;

https://reviewboard.mozilla.org/r/87616/#review88242
Attachment #8803306 - Flags: review?(jwwang) → review+
Comment on attachment 8805091 [details]
Bug 1309516 part 4 - always notify LoadedMetadataEvent before decoding first frame;

https://reviewboard.mozilla.org/r/88900/#review88244
Attachment #8805091 - Flags: review?(jwwang) → review+
Comment on attachment 8803308 [details]
Bug 1309516 part 5 - remove unused MediaDecoderReaderWrapper::AwaitStartTime();

https://reviewboard.mozilla.org/r/87620/#review88248
Attachment #8803308 - Flags: review?(jwwang) → review+
Comment on attachment 8803309 [details]
Bug 1309516 part 6 - remove unused MediaDecoderReaderWrapper::mStartTimeRendezvous;

https://reviewboard.mozilla.org/r/87622/#review88250
Attachment #8803309 - Flags: review?(jwwang) → review+
Comment on attachment 8805092 [details]
Bug 1309516 part 7 - modify the seek operation;

https://reviewboard.mozilla.org/r/88902/#review88252

::: dom/media/MediaDecoderStateMachine.cpp:1490
(Diff revision 1)
>  {
>    int64_t seekTime = mSeekTask->GetSeekTarget().GetTime().ToMicroseconds();
> -  int64_t newCurrentTime = seekTime;
> +  int64_t newCurrentTime;
>  
> -  // Setup timestamp state.
>    RefPtr<MediaData> video = mMaster->VideoQueue().PeekFront();

'video' can be moved into the else block.

::: dom/media/MediaDecoderStateMachine.cpp:1491
(Diff revision 1)
>    int64_t seekTime = mSeekTask->GetSeekTarget().GetTime().ToMicroseconds();
> -  int64_t newCurrentTime = seekTime;
> +  int64_t newCurrentTime;
>  
> -  // Setup timestamp state.
>    RefPtr<MediaData> video = mMaster->VideoQueue().PeekFront();
> -  if (seekTime == mMaster->Duration().ToMicroseconds()) {
> +  if (mSeekTask->GetSeekTarget().IsAccurate()) {

Please add comments about how newCurrentTime is calculated and its reasonale.
Attachment #8805092 - Flags: review?(jwwang) → review+
Comment on attachment 8805093 [details]
Bug 1309516 part 8 - modify MDSM::RecomputeDuration();

https://reviewboard.mozilla.org/r/88904/#review88254

::: dom/media/MediaDecoderStateMachine.cpp:2381
(Diff revision 1)
>      // We don't fire duration changed for this case because it should have
>      // already been fired on the main thread when the explicit duration was set.
>      duration = TimeUnit::FromSeconds(d);
>    } else if (mEstimatedDuration.Ref().isSome()) {
>      duration = mEstimatedDuration.Ref().ref();
> -  } else if (Info().mMetadataDuration.isSome()) {
> +  } else if (mInfo.isSome() && Info().mMetadataDuration.isSome()) {

How could we call RecomputeDuration() becuase metadata is loaded?
Attachment #8805093 - Flags: review?(jwwang) → review+
Comment on attachment 8805090 [details]
Bug 1309516 part 1 - retrieve start time before resolving the metadata promise;

https://reviewboard.mozilla.org/r/88898/#review88224

> Why not use Move here ?

The type of MetadataHolder::mTags is nsAutoPtr<MetadataTags> which is different from the MFR::mTags which is UniquePtr<MetadataTags>. I could make them consistent in a following bug, and then apply move semantics.

> s/Used/Use
> 
> but you're not using mStartTime anywhere, and it's already in mInfo so you can drop this unused member.

Hmm..., this is the decleration of mInfo's mStartTime (we are now in the MediaInfo.h), so I think I need to keep this memebr. If you're talking about MediaDecoder::mStartTime, then plan to remove it at the following bugs, I would like to remove the legacy ogg reader's code first and then remove MediaDecoder::DispatchSetStartTime() and MediaDecoder::mStartTime.
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c9ab7d66ca

Thanks for the review!
Keywords: checkin-needed
Blocks: 1313632
Blocks: 1313635
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90409a433f31
part 1 - retrieve start time before resolving the metadata promise; r=jya
https://hg.mozilla.org/integration/autoland/rev/970694b0b279
part 2 - replace MediaFormatReader::DemuxStartTime() with MediaInfo::mStartTime; r=jya
https://hg.mozilla.org/integration/autoland/rev/e39be827b220
part 3 - make MediaDecoderReaderWrapper keeps the start time returned from reader;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/35d6e08883d6
part 4 - always notify LoadedMetadataEvent before decoding first frame;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a154fa107dd3
part 5 - remove unused MediaDecoderReaderWrapper::AwaitStartTime();r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a876261d2d38
part 6 - remove unused MediaDecoderReaderWrapper::mStartTimeRendezvous;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/9ddc65900391
part 7 - modify the seek operation;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5cb98008b3e3
part 8 - modify MDSM::RecomputeDuration();r=jwwang
Keywords: checkin-needed
Comment on attachment 8806256 [details]
Bug 1309516 part 9 - make sure that MDSM::mDuration is always assigned once we have meatadata;

https://reviewboard.mozilla.org/r/89770/#review89198
Attachment #8806256 - Flags: review?(jwwang) → review+
Keywords: checkin-needed
needs rebasing for autoland 

 conflicts while merging dom/media/MediaFormatReader.h! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(kaku)
Keywords: checkin-needed
Rebased, please help to check-in again, thanks!
Flags: needinfo?(kaku)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d460f4c4b0c0
part 1 - retrieve start time before resolving the metadata promise; r=jya
https://hg.mozilla.org/integration/autoland/rev/3983dd7e87bb
part 2 - replace MediaFormatReader::DemuxStartTime() with MediaInfo::mStartTime; r=jya
https://hg.mozilla.org/integration/autoland/rev/0699225db846
part 3 - make MediaDecoderReaderWrapper keeps the start time returned from reader;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/804308421d74
part 4 - always notify LoadedMetadataEvent before decoding first frame;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/874207ad3984
part 5 - remove unused MediaDecoderReaderWrapper::AwaitStartTime();r=jwwang
https://hg.mozilla.org/integration/autoland/rev/66f39ef1bdce
part 6 - remove unused MediaDecoderReaderWrapper::mStartTimeRendezvous;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3686da2395b9
part 7 - modify the seek operation;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/cb6854f262a0
part 8 - modify MDSM::RecomputeDuration();r=jwwang
https://hg.mozilla.org/integration/autoland/rev/9d1487ccc2e3
part 9 - make sure that MDSM::mDuration is always assigned once we have meatadata; r=jwwang
Keywords: checkin-needed
Backed out for notrun errors in media-src/media-src-7_3.html web-platform-test on Windows 8 x64:

https://hg.mozilla.org/integration/autoland/rev/85848a6d19ab9897632380238f3498db07b6f73f
https://hg.mozilla.org/integration/autoland/rev/d284ffa5e67381e5a256f9a90d99c0d4ab116913
https://hg.mozilla.org/integration/autoland/rev/e94d188473d3063b9fdf3b577734b7e12a661a2a
https://hg.mozilla.org/integration/autoland/rev/2b7cc43dfb60c65243665be25aed8a65b5740508
https://hg.mozilla.org/integration/autoland/rev/1e1e71cf9feac94a4626178313022af9a128537f
https://hg.mozilla.org/integration/autoland/rev/7244d8f6c4f30d02b5d16c8e73513a457081a228
https://hg.mozilla.org/integration/autoland/rev/420731164890399f40d6e657533708ca46b5a657
https://hg.mozilla.org/integration/autoland/rev/5486a88515be99f38ae19d55f724cd0278b16eea
https://hg.mozilla.org/integration/autoland/rev/ed0779136aa6df31f125e7fe714524c3436ad2fd

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d1487ccc2e34cecc2ee040b81d5ea164b57e964
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6189616&repo=autoland

02:15:10     INFO - TEST-START | /content-security-policy/media-src/media-src-7_3.html
02:15:21     INFO - PROCESS | 2532 | MARIONETTE LOG: INFO: Timeout fired
02:15:21     INFO - 
02:15:21     INFO - TEST-UNEXPECTED-NOTRUN | /content-security-policy/media-src/media-src-7_3.html | In-policy track element - expected PASS
02:15:21     INFO - TEST-UNEXPECTED-TIMEOUT | /content-security-policy/media-src/media-src-7_3.html | expected OK
Flags: needinfo?(kaku)
So, I finally realized what happened...

In the /testing/web-platform/tests/content-security-policy/media-src/media-src-7_3.html, the test case loads a MP4 file (testing/web-platform/tests/media/white.mp4) and listens to the "loadeddata event" which is never filed so that timed out.

The root cause is that the first decoded video data has time stamp -33 which is considered as invalid here (http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/dom/media/mediasink/VideoSink.cpp#368) so that we don't set the video element's image container. In this way, in the HTMLMediaElement::UpdateReadyStateInternal(), we are not able to transition the readyState from HAVE_METADATA to HAVE_CURRENT_DATA (http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/dom/html/HTMLMediaElement.cpp#4781), which is the criteria of dispatching the "loadeddata event".

However, what is interesting is that the demuxed video frames are actually stated from 0 (the sequence is 0, 133333, 66666, 33333, 100000, 266666, 200000, 166666, 233333, 400000, 333333, 300000, 366666, 533333, 466666, ...), but the 1st decoded frame has time stamp -33. 

@jya, do you have any idea about this phenomenon? why does the Windows decoder change the time stamp of demuxed data? BTW, the phenomenon only happens on Windows (I could reproduce it on Windows8 and Windows10).
Flags: needinfo?(kaku) → needinfo?(jyavenard)
A negative timestamp isn't invalid.
Isn't the time being reworked however so that the MediaData::mTime is the originalMediaData::mTime - StartTime() ?

Which platform is this on? my guess is windows. The Windows MFT rework the sample time, it's an absolute pain to know what's going to come out.
But I've never seen video frames coming out negative when the input time was 0.

I'll look at the video tomorrow and report back.
Flags: needinfo?(jyavenard)
Comment on attachment 8810771 [details]
Bug 1309516 part 0 - make sure that the VideoSink::Redraw() always draws something;

https://reviewboard.mozilla.org/r/93064/#review93008

::: dom/media/mediasink/VideoSink.cpp:293
(Diff revision 1)
>    if (!aInfo.IsValid() || !mContainer) {
>      return;
>    }
>  
>    if (VideoQueue().GetSize() > 0) {
> -    RenderVideoFrames(1);
> +    VideoData* frame = VideoQueue().Peek()->As<VideoData>();

1. You should call PeekFront() instead of Peek().
2. It is not safe because |frame| could become a dangling pointer if the video queue is modified on another thread.
3. You can do:

RefPtr<MediaData> v = VideoQueue().PeekFront();
if (v) {
  // do something.
}

So VideoQueue (which has an internal lock) is only accessed once.
Attachment #8810771 - Flags: review?(jwwang) → review-
Comment on attachment 8810771 [details]
Bug 1309516 part 0 - make sure that the VideoSink::Redraw() always draws something;

https://reviewboard.mozilla.org/r/93064/#review93076
Attachment #8810771 - Flags: review?(jwwang) → review+
Comment on attachment 8810771 [details]
Bug 1309516 part 0 - make sure that the VideoSink::Redraw() always draws something;

https://reviewboard.mozilla.org/r/93064/#review93342

::: dom/media/mediasink/VideoSink.cpp:292
(Diff revision 2)
>    // No video track, nothing to draw.
>    if (!aInfo.IsValid() || !mContainer) {
>      return;
>    }
>  
> -  if (VideoQueue().GetSize() > 0) {
> +  RefPtr<VideoData> frame = VideoQueue().PeekFront()->As<VideoData>();

Note you can do:

RefPtr<MediaData> v = VideoQueue().PeekFront();
if (v) {
  VideoData* frame = v->As<VideoData>();
  // do something.
}

to save a AddRef/Release pair.
Comment on attachment 8810771 [details]
Bug 1309516 part 0 - make sure that the VideoSink::Redraw() always draws something;

https://reviewboard.mozilla.org/r/93064/#review93342

> Note you can do:
> 
> RefPtr<MediaData> v = VideoQueue().PeekFront();
> if (v) {
>   VideoData* frame = v->As<VideoData>();
>   // do something.
> }
> 
> to save a AddRef/Release pair.

Actually, the original code might crash since we might invoke ->as<VideoData>() at nullptr, try told me that.
Comment on attachment 8805092 [details]
Bug 1309516 part 7 - modify the seek operation;

@JW, I modified the mechanism of calculating new current time according to our discussion on Friday. Please have another look!
Attachment #8805092 - Flags: review+ → review?(jwwang)
Comment on attachment 8805092 [details]
Bug 1309516 part 7 - modify the seek operation;

https://reviewboard.mozilla.org/r/88902/#review94414

LGTM.
Attachment #8805092 - Flags: review?(jwwang) → review+
Thanks for the review!

The test fail on Linux-debug seems has nothing to do with these patches because it also happens on the central without these patches. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bbf6c7617ad60f0c7e8ca501b35d60274bc7adf&selectedJob=31506548)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7a9344abdb75
part 0 - make sure that the VideoSink::Redraw() always draws something; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fe6765272b16
part 1 - retrieve start time before resolving the metadata promise; r=jya
https://hg.mozilla.org/integration/autoland/rev/2a1429d05eff
part 2 - replace MediaFormatReader::DemuxStartTime() with MediaInfo::mStartTime; r=jya
https://hg.mozilla.org/integration/autoland/rev/cfb8fea13412
part 3 - make MediaDecoderReaderWrapper keeps the start time returned from reader;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/08a0fc71368e
part 4 - always notify LoadedMetadataEvent before decoding first frame;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/69271eb314a9
part 5 - remove unused MediaDecoderReaderWrapper::AwaitStartTime();r=jwwang
https://hg.mozilla.org/integration/autoland/rev/862b479c1d2f
part 6 - remove unused MediaDecoderReaderWrapper::mStartTimeRendezvous;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c2544387444f
part 7 - modify the seek operation;r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5f4e5f0c9606
part 8 - modify MDSM::RecomputeDuration();r=jwwang
https://hg.mozilla.org/integration/autoland/rev/efb08fa7d183
part 9 - make sure that MDSM::mDuration is always assigned once we have meatadata; r=jwwang
Keywords: checkin-needed
Depends on: 1319035
Blocks: 1324357
Depends on: 1329480
No longer depends on: 1329480
Depends on: 1496567
Regressions: 1657128
Regressions: 1850851
You need to log in before you can comment on or make changes to this bug.