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

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Depends on 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(10 attachments, 3 obsolete attachments)

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
Assignee

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → kaku
Assignee

Updated

3 years ago
Depends on: 1309517
Assignee

Comment 1

3 years ago
: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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

3 years ago
Here are the WIP patches, not pass the try server yet. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=929e24e0781d
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8803304 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8803307 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8803310 - Attachment is obsolete: true
Assignee

Comment 21

3 years ago
mozreview-review
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)
Assignee

Updated

3 years ago
Attachment #8805090 - Flags: review?(kaku)

Comment 22

3 years ago
mozreview-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

::: 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 23

3 years ago
mozreview-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 24

3 years ago
mozreview-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 25

3 years ago
mozreview-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 26

3 years ago
mozreview-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 27

3 years ago
mozreview-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 28

3 years ago
mozreview-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 29

3 years ago
mozreview-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+
Assignee

Comment 30

3 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 39

3 years ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c9ab7d66ca

Thanks for the review!
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Updated

3 years ago
Blocks: 1313632
Assignee

Updated

3 years ago
Blocks: 1313635

Comment 40

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

3 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 65

3 years ago
Rebased, please help to check-in again, thanks!
Flags: needinfo?(kaku)
Keywords: checkin-needed

Comment 66

3 years ago
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)
Assignee

Comment 68

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 80

3 years ago
mozreview-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/#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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 91

3 years ago
mozreview-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 92

3 years ago
mozreview-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.
Assignee

Comment 93

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 115

3 years ago
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 117

3 years ago
mozreview-review
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+
Assignee

Comment 118

3 years ago
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

Comment 119

3 years ago
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
Assignee

Updated

2 years ago
Blocks: 1324357

Updated

2 years ago
Depends on: 1329480

Updated

2 years ago
No longer depends on: 1329480

Updated

8 months ago
Depends on: 1496567
You need to log in before you can comment on or make changes to this bug.