Closed
Bug 1100776
Opened 10 years ago
Closed 10 years ago
Reference-count MediaData
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
112.45 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
The current AutoPtr mechanism is pretty error prone. See bug 1097823 comment 7. I'll whip up a patch.
Assignee | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=213413cd2bd1
Assignee | ||
Comment 2•10 years ago
|
||
Let's see how many try pushes it takes for me to get this green on all the platforms I can't build locally: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7aa9ed66994
Assignee | ||
Comment 3•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f2cbaa3e68ec
Assignee | ||
Comment 4•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=854339dfdbe1
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8525094 -
Flags: review?(cpearce)
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=52447e7622da https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=488220fdc416
Comment 7•10 years ago
|
||
Comment on attachment 8525094 [details] [diff] [review] Reference-count MediaData. v1 Review of attachment 8525094 [details] [diff] [review]: ----------------------------------------------------------------- Great! I'm a bit dubious about MediaQueueDeallocator, and there are build failures on your Try push. r+ with those fixed. ::: dom/media/MediaData.h @@ +81,5 @@ > , mChannels(aChannels) > , mRate(aRate) > , mAudioData(aData) > { > MOZ_COUNT_CTOR(AudioData); We should be able to remove the MOZ_COUNT_CTOR/DTOR here, the refcnt is logged in NS_INLINE_DECL_THREADSAFE_REFCOUNTING. ::: dom/media/MediaDecoderStateMachine.cpp @@ +489,5 @@ > // badly muxed resources. > if (!a || a->GetEndTime() >= minLastAudioPacketTime) > break; > OnAudioEndTimeUpdate(std::max(mAudioEndTime, a->GetEndTime())); > + nsRefPtr<AudioData> forgetMe = AudioQueue().PopFront(); probably best to call this "deleteMe" rather than "forgetMe", since you actually want to delete it, not forget it. @@ +2810,5 @@ > currentFrame->mTime, clock_time, ++droppedFrames); > } > #endif > currentFrame = frame; > + nsRefPtr<VideoData> forgetMe = VideoQueue().PopFront(); deleteMe ? ::: dom/media/MediaQueue.h @@ +17,5 @@ > // Thread and type safe wrapper around nsDeque. > template <class T> > class MediaQueueDeallocator : public nsDequeFunctor { > virtual void* operator() (void* aObject) { > + nsRefPtr<T> forgetMe = dont_AddRef(static_cast<T*>(aObject)); This doesn't make sense to me. It's a no-op right? It wraps aObject in a refptr, without addreffing, so refcnt doesn't increment, and then forget()s, so the refcnt doesn't decrement. Isn't this function supposed to delete aObject? Maybe it should NS_RELEASE instead? Or if this really should be a no-op, then it should be empty with a comment explaining why. ::: dom/media/ogg/OggReader.cpp @@ +1539,5 @@ > // that we may have already decoded, and discard frames up to the > // keyframe. > VideoData* v; > while ((v = mVideoQueue.PeekFront()) && !v->mKeyframe) { > + nsRefPtr<VideoData> forgetMe = mVideoQueue.PopFront(); deleteMe
Attachment #8525094 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #7) > there are build failures on your Try push. Yeah they're fixed in the second try push in comment 6. > We should be able to remove the MOZ_COUNT_CTOR/DTOR here, the refcnt is > logged in NS_INLINE_DECL_THREADSAFE_REFCOUNTING. Good catch. > probably best to call this "deleteMe" rather than "forgetMe", since you > actually want to delete it, not forget it. Well, in a refcounted world it's not really a delete either. I think "releaseMe" is the most appropriate here (I'll do that). > ::: dom/media/MediaQueue.h > @@ +17,5 @@ > > // Thread and type safe wrapper around nsDeque. > > template <class T> > > class MediaQueueDeallocator : public nsDequeFunctor { > > virtual void* operator() (void* aObject) { > > + nsRefPtr<T> forgetMe = dont_AddRef(static_cast<T*>(aObject)); > > This doesn't make sense to me. It's a no-op right? It wraps aObject in a > refptr, without addreffing, so refcnt doesn't increment, and then forget()s, > so the refcnt doesn't decrement. Doh, the forget() was spurious. Good catch.
Assignee | ||
Comment 9•10 years ago
|
||
Final all-platform try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=508882290174
Assignee | ||
Comment 10•10 years ago
|
||
Followup try push for minor build bustage: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5cf3b3c35852 All green. Pushed to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/b49cba7d3b28
https://hg.mozilla.org/mozilla-central/rev/b49cba7d3b28
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•