Closed
Bug 1250054
Opened 8 years ago
Closed 8 years ago
Create a wrapper for start time adjustment for MDSM
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(2 files, 8 obsolete files)
Per bug 1244477 comment 7, we can create a wrapper around existing readers for adjusting timestamps of {Audio|Video}Data so MDSM can always deal with adjusted timestamps. It also makes the code of MDSM less confusing since we don't have to worry about whether or when to take start time into account.
Updated•8 years ago
|
Priority: -- → P2
Summary: Create a wrapper for star time adjustment for MDSM → Create a wrapper for start time adjustment for MDSM
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jwwang
Assignee | ||
Comment 1•8 years ago
|
||
The work involves promise chaining and can't continue until bug 1250829 is fixed.
Depends on: 1250829
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41929/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41929/
Attachment #8733736 -
Flags: review?(jyavenard)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41931/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41931/
Attachment #8733737 -
Flags: review?(jyavenard)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41933/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41933/
Attachment #8733738 -
Flags: review?(jyavenard)
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41935/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41935/
Attachment #8733739 -
Flags: review?(jyavenard)
Comment 6•8 years ago
|
||
Comment on attachment 8733736 [details] MozReview Request: Bug 1250054. Part 1 - add new files. r=jya. https://reviewboard.mozilla.org/r/41929/#review39181 i dont see the point of this commit. it serves no purpose
Attachment #8733736 -
Flags: review?(jyavenard)
Comment 7•8 years ago
|
||
Comment on attachment 8733737 [details] MozReview Request: Bug 1250054. Part 2 - move StartTimeRendezvous from MediaDecoderStateMachine.h to MediaDecoderReaderWrapper.cpp. r=jya. https://reviewboard.mozilla.org/r/41931/#review39183 Combine P1 with P2
Attachment #8733737 -
Flags: review?(jyavenard) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8733737 [details] MozReview Request: Bug 1250054. Part 2 - move StartTimeRendezvous from MediaDecoderStateMachine.h to MediaDecoderReaderWrapper.cpp. r=jya. https://reviewboard.mozilla.org/r/41931/#review39185 ::: dom/media/MediaDecoderReaderWrapper.cpp:11 (Diff revision 1) > > #include "MediaDecoderReaderWrapper.h" > > namespace mozilla { > > +extern LazyLogModule gMediaDecoderLog; Where is the header for this? This wont compile on its own
Attachment #8733737 -
Flags: review+
Comment 9•8 years ago
|
||
Comment on attachment 8733738 [details] MozReview Request: Bug 1250054. Part 3 - implement MediaDecoderReaderWrapper. r=jya. https://reviewboard.mozilla.org/r/41933/#review39187 I really dont see the benefits of moving this out of the MDSM. you arent abstracting anything, nor make things simpler to read or maintain IMHO .. do we really need this? ::: dom/media/MediaDecoderReaderWrapper.h:62 (Diff revision 1) > + const bool mForceZeroStartTime; > + const RefPtr<AbstractThread> mOwnerThread; > + const RefPtr<MediaDecoderReader> mReader; > + > + bool mShutdown = false; > + RefPtr<StartTimeRendezvous> mStartTimeRendezvous; If you disabled unified build it would cause problem if you only forward declared StartTimeRendezVous like this as it doesnt have the declaration for the destructor
Attachment #8733738 -
Flags: review?(jyavenard)
Comment 10•8 years ago
|
||
Comment on attachment 8733739 [details] MozReview Request: Bug 1250054. Part 4 - employ MediaDecoderReaderWrapper and remove code about adjusting start time. r=jya. https://reviewboard.mozilla.org/r/41935/#review39191 Upon seeing P4, maybe it is simpler that way...
Attachment #8733739 -
Flags: review?(jyavenard) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8733738 [details] MozReview Request: Bug 1250054. Part 3 - implement MediaDecoderReaderWrapper. r=jya. https://reviewboard.mozilla.org/r/41933/#review39193
Attachment #8733738 -
Flags: review+
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/41933/#review39195 ::: dom/media/MediaDecoderReaderWrapper.h:45 (Diff revision 1) > + int64_t StartTime() const; > + RefPtr<MetadataPromise> ReadMetadata(); > + RefPtr<HaveStartTimePromise> AwaitStartTime(); > + RefPtr<AudioDataPromise> RequestAudioData(); > + RefPtr<VideoDataPromise> RequestVideoData(bool aSkipToNextKeyframe, > + int64_t aTimeThreshold); Would be a good time to use TimeUnit while at it
Updated•8 years ago
|
Attachment #8733737 -
Flags: review+
Comment 13•8 years ago
|
||
Comment on attachment 8733737 [details] MozReview Request: Bug 1250054. Part 2 - move StartTimeRendezvous from MediaDecoderStateMachine.h to MediaDecoderReaderWrapper.cpp. r=jya. https://reviewboard.mozilla.org/r/41931/#review39197
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #12) > https://reviewboard.mozilla.org/r/41933/#review39195 > > ::: dom/media/MediaDecoderReaderWrapper.h:45 > (Diff revision 1) > > + int64_t StartTime() const; > > + RefPtr<MetadataPromise> ReadMetadata(); > > + RefPtr<HaveStartTimePromise> AwaitStartTime(); > > + RefPtr<AudioDataPromise> RequestAudioData(); > > + RefPtr<VideoDataPromise> RequestVideoData(bool aSkipToNextKeyframe, > > + int64_t aTimeThreshold); > > Would be a good time to use TimeUnit while at it Will do.
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/15058cd33731 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe425b8cb8cb https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3d6ace0405
Comment 16•8 years ago
|
||
backed this out since this seems have caused leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=24793567&repo=mozilla-inbound
Flags: needinfo?(jwwang)
Comment 17•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c281e36bb637 https://hg.mozilla.org/integration/mozilla-inbound/rev/dfda3d7872c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/97001a5fbcb5
Comment 18•8 years ago
|
||
I should note that patch 1 a) wasn't r+'d, and b) the landed (and backed-out) patch doesn't match what's in mozreview; it looks like a refactor was done before landing.
Comment 19•8 years ago
|
||
To be fair, I had changed my mind in regards to the 2nd patch (which was merge with the first one) and asked JW to redo it using TimeUnit which he did. I don't think the refactor was significant however and at a glance I can't see how this could have lead to leaked.
Assignee | ||
Comment 20•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43373/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43373/
Attachment #8736546 -
Flags: review?(jyavenard)
Attachment #8736547 -
Flags: review?(jyavenard)
Attachment #8736548 -
Flags: review?(jyavenard)
Attachment #8736549 -
Flags: review?(jyavenard)
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43375/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43375/
Assignee | ||
Comment 22•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43377/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43377/
Assignee | ||
Comment 23•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43379/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43379/
Assignee | ||
Updated•8 years ago
|
Attachment #8733736 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8733737 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8733738 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8733739 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
This is why I hesitate to upload revised patches to review board again because somehow it screws the review history...
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jwwang)
Attachment #8736546 -
Flags: review?(jyavenard) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8736547 -
Flags: review?(jyavenard) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8736548 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 25•8 years ago
|
||
Try without part 4: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9cc87b7cfab with part 4: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5548549f7ce Part 4 seems to fix the leaks.
Comment 26•8 years ago
|
||
https://reviewboard.mozilla.org/r/43373/#review40001 ::: dom/media/MediaDecoderReaderWrapper.h:18 (Diff revision 1) > + > +#include "MediaDecoderReader.h" > + > +namespace mozilla { > + > +class StartTimeRendezvous; How could this compile on its own if you don't define StartTimeRendezVous in the header? this will only compile if you have unified build on. So that can't go as-is. You need to have proper definition in the header.
Updated•8 years ago
|
Attachment #8736546 -
Flags: review+
Assignee | ||
Comment 27•8 years ago
|
||
https://reviewboard.mozilla.org/r/43373/#review40001 > How could this compile on its own if you don't define StartTimeRendezVous in the header? > > this will only compile if you have unified build on. > > So that can't go as-is. > > You need to have proper definition in the header. To use |RefPtr<StartTimeRendezvous> mStartTimeRendezvous| in the header, you don't need the complete definition. A forward declaration will do.
Comment 28•8 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #27) > To use |RefPtr<StartTimeRendezvous> mStartTimeRendezvous| in the header, you > don't need the complete definition. A forward declaration will do. I'm referring to the use in MediaDecoderStateMachine. you are using methods from the StartTimeRendezvous like in MediaDecoderStateMachine::RequestAudioData() I understand that this part will be removed later, but in the mean time, P1 on its own will not compile unless unified build is on, and actually it may not even compile with unified build depending on how the builder have grouped files.
Assignee | ||
Comment 29•8 years ago
|
||
It will compile and work only after applying all the patches. Sometimes it is hard to make each patch compile and run correctly on its own. I won't spend efforts in it until it is a pre-requisite to get an r+.
Comment 30•8 years ago
|
||
Then you shouldn't do an intermediary patch. To me you should be able to checkout anywhere in the tree and be able to compile at any time and not rely on a future patch. What's the point otherwise to break down patches in multiple commits? Otherwise it makes later bisections difficult as suddenly you have invalid commit known to be broken. So please, merge the patches together, or properly have the declaration in the header so it can be used in the MDSM (and MDSM will also need to include the header)
Assignee | ||
Comment 31•8 years ago
|
||
Small patches are easier to review as long as their intention is clear (maybe won't compile though). For a large bug, it is not feasible to expect each commit will compile and run correctly on its own as far as bisect is concerned. Moreover, I think bisect point can not be picked randomly. It should be a checkpoint where all patches of a bug are applied. Anyway, I will merge patches to ensure each commit will compile.
Comment 32•8 years ago
|
||
Yes, small patch are easier to review, and I've personally always taken great care so that any of them on their own will compile. And AFAIK, all people in the media team have always done it that way. Having said that here, it's really not difficult to make P1 work on its own. It would even make the review easier imho as you just moved it from one header to another...
Assignee | ||
Updated•8 years ago
|
Attachment #8736546 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8736547 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8736548 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8736549 -
Attachment is obsolete: true
Attachment #8736549 -
Flags: review?(jyavenard)
Assignee | ||
Comment 33•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43451/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43451/
Attachment #8736613 -
Flags: review?(jyavenard)
Attachment #8736614 -
Flags: review?(jyavenard)
Assignee | ||
Comment 34•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43453/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43453/
Comment 35•8 years ago
|
||
Comment on attachment 8736613 [details] MozReview Request: Bug 1250054. Part 1 - implement MediaDecoderReaderWrapper. r=jya. https://reviewboard.mozilla.org/r/43451/#review40015 This would fail to compile too (actually linkage error) because you now have two classes with the same name
Attachment #8736613 -
Flags: review?(jyavenard) → review+
Comment 36•8 years ago
|
||
Comment on attachment 8736614 [details] MozReview Request: Bug 1250054. Part 2 - employ MediaDecoderReaderWrapper for MDSM and remove code about adjusting start time. r=jya. https://reviewboard.mozilla.org/r/43453/#review40017
Attachment #8736614 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 37•8 years ago
|
||
No it won't because the namespace is different. P1 alone compiles in both unified and non-unified builds.
Comment 38•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f5d08d29a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/2605e7e25764
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6f5d08d29a0 https://hg.mozilla.org/mozilla-central/rev/2605e7e25764
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•