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)
Core
Audio/Video: Playback
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 | ||
Updated•8 years ago
|
Assignee: nobody → kaku
Assignee | ||
Comment 1•8 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)
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 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) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8803304 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8803307 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8803310 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffdc9733c9fc
Assignee | ||
Comment 21•8 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•8 years ago
|
Attachment #8805090 -
Flags: review?(kaku)
Comment 22•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c9ab7d66ca Thanks for the review!
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 40•8 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 41•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/fb72036436f98ac04e6290c0c481e01c30257e5d for Win8 wpt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5866503&repo=autoland
Updated•8 years ago
|
Priority: -- → P3
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 51•8 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 | ||
Comment 53•8 years ago
|
||
Try looks good again... https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9767b6da89f537f7064c65ad296e2f276b3476
Assignee | ||
Comment 54•8 years ago
|
||
Web platform tests looks okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7388851a126
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 55•8 years ago
|
||
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•8 years ago
|
||
Rebased, please help to check-in again, thanks!
Flags: needinfo?(kaku)
Keywords: checkin-needed
Comment 66•8 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
Comment 67•8 years ago
|
||
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•7 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)
Comment 69•7 years ago
|
||
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•7 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•7 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•7 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•7 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) |
Assignee | ||
Comment 104•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a58878082c3929b0b0f4cf02eb1ac6943780c29f Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f242a218e75204cb3b4d7c32d3eea318d37a382
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•7 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)
Assignee | ||
Comment 116•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b362ab8c31b404faec68a0375dc77953bcd1363 try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96aa0dec52bd518ea1a5af2feb3270e013d448fd
Comment 117•7 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•7 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•7 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
Comment 120•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a9344abdb75 https://hg.mozilla.org/mozilla-central/rev/fe6765272b16 https://hg.mozilla.org/mozilla-central/rev/2a1429d05eff https://hg.mozilla.org/mozilla-central/rev/cfb8fea13412 https://hg.mozilla.org/mozilla-central/rev/08a0fc71368e https://hg.mozilla.org/mozilla-central/rev/69271eb314a9 https://hg.mozilla.org/mozilla-central/rev/862b479c1d2f https://hg.mozilla.org/mozilla-central/rev/c2544387444f https://hg.mozilla.org/mozilla-central/rev/5f4e5f0c9606 https://hg.mozilla.org/mozilla-central/rev/efb08fa7d183
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•