Closed Bug 1100776 Opened 10 years ago Closed 10 years ago

Reference-count MediaData

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

The current AutoPtr mechanism is pretty error prone. See bug 1097823 comment 7. I'll whip up a patch.
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
Attachment #8525094 - Flags: review?(cpearce)
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/b49cba7d3b28
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1102658
You need to log in before you can comment on or make changes to this bug.