1055114 - remove nsAutoRef usages from content/media/gmp/ files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Nathan Froyd [:froydnj]]

In bug 1055035, we're trying to remove nsAutoRef.h.  Specifically for this bug, we're trying to remove nsAutoRef, which can be replaced by mozilla::UniquePtr, superior in many ways.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

I think this makes more sense than simply substituting UniquePtr for nsAutoRef;
makes the flow of ownership more explicit in the code.  We could make things
even better, IMHO, by making GMPVideoHost::CreateFrame return UniquePtrs, so
that it's more obvious that the caller will take ownership.  But that's a
separate patch...

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8474659 [details] [diff] [review]
convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

Review of attachment 8474659 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/fmp4/eme/EMEH264Decoder.cpp
@@ +317,5 @@
>    frame->SetFrameType(sample->is_sync_point ? kGMPKeyFrame : kGMPDeltaFrame);
>    frame->SetBufferType(GMP_BufferLength32);
>  
>    nsTArray<uint8_t> info; // No codec specific per-frame info to pass.
> +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame), false, info, 0);

Shouldn't the UniquePtr be created up around line 300, where the frame is created? Otherwise the pointer passed to Decode() isn't unique?

I assume the copy ctor of UniquePtr doesn't destroy the thing being pointed to, like nsAutoPtr?

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +635,5 @@
>    nsTArray<uint8_t> codecSpecificInfo;
>    codecSpecificInfo.AppendElements((uint8_t*)&info, sizeof(GMPCodecSpecificInfo));
>  
>    LOGD(("GMP Decode: %llu, len %d", frame->TimeStamp(), aInputImage._length));
> +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame),

Same comment as I made in EMEH264Decoder applies here.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Chris Pearce (:cpearce) from comment #2)
> Comment on attachment 8474659 [details] [diff] [review]
> convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where
> appropriate
> 
> Review of attachment 8474659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/fmp4/eme/EMEH264Decoder.cpp
> @@ +317,5 @@
> >    frame->SetFrameType(sample->is_sync_point ? kGMPKeyFrame : kGMPDeltaFrame);
> >    frame->SetBufferType(GMP_BufferLength32);
> >  
> >    nsTArray<uint8_t> info; // No codec specific per-frame info to pass.
> > +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame), false, info, 0);
> 
> Shouldn't the UniquePtr be created up around line 300, where the frame is
> created? Otherwise the pointer passed to Decode() isn't unique?

We could certainly do that; that was part of what I was referring to when I said it'd be better if CreateFrame returned a UniquePtr, either:

  UniquePtr<GMPVideoFrame> CreateFrame(GMPVideoFrameFormat);

or:

  GMPErr CreateFrame(GMPVideoFrameFormat, UniquePtr<GMPVideoFrame>&);

I like the first one better, personally, but I can see why the second is preferred.

You'd have to play some games with UniquePtr in the body of the function, given the casts that take place.  I'm willing to do something like that in a followup patch, but I thought I'd keep this one as simple as possible.

> I assume the copy ctor of UniquePtr doesn't destroy the thing being pointed
> to, like nsAutoPtr?

UniquePtr doesn't have a copy ctor, it just has a move ctor.  And the move ctor does what you'd expect; i.e. passes ownership.  The move-ctor-ness is generally much more obvious than the copy-ctor-ness, though, as well as being closer to what is actually happening.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8474659 [details] [diff] [review]
convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

Review of attachment 8474659 [details] [diff] [review]:
-----------------------------------------------------------------

Please make the change to stick the return value of CreateFrame in the UniquePtr as soon as you can. Otherwise, you're using UniquePtr semantically as an nsAutoPtr. No need to change the interface of GMPVideoHost, that would be tricky as that interface is shared with plugins in the child process too, just make the |frame| variable a UniquePtr.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

This replacement patch moves the UniquePtr bits up farther, as requested.  Not
really that much more difficult, but tedious in trying to get the DefaultDelete
specializations at the correct points.

Some of the choices made in the gmp code seem unusual, but I'm completely
unfamiliar with this code and any decision-making behind it.  Would you mind
answering some questions purely for my own curiousity?

1. Are the gmp-* files supposed to be standalone, without reference to the rest
   of the Mozilla source tree?
2. If so, why are we sticking them in EXPORTS?
3. Why use |virtual Destroy()| for frames, when virtual destructors would have
   done just as well and integrated better with C++?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Created attachment 8475996 [details] [diff] [review]
> convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where
> appropriate
> 
> This replacement patch moves the UniquePtr bits up farther, as requested. 
> Not
> really that much more difficult, but tedious in trying to get the
> DefaultDelete
> specializations at the correct points.
> 
> Some of the choices made in the gmp code seem unusual, but I'm completely
> unfamiliar with this code and any decision-making behind it.  Would you mind
> answering some questions purely for my own curiousity?
> 
> 1. Are the gmp-* files supposed to be standalone, without reference to the
> rest
>    of the Mozilla source tree?

Yes, they're for makers of Gecko Media Plugins to use to write a plugin. Used by OpenH264 plugin, and EME plugins.

> 2. If so, why are we sticking them in EXPORTS?

Uh, you mean in the moz.build? So that the WebRTC and media code in other directories can #include them?

> 3. Why use |virtual Destroy()| for frames, when virtual destructors would
> have
>    done just as well and integrated better with C++?

These interfaces are used in the child process. There's no guarantee that the child process has the same delete operator, so we can't assume `delete object` will Just Work in the child process.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8475996 [details] [diff] [review]
convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

Review of attachment 8475996 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with nits picked and consistent passing of ownership.

We should be consistent when using these, and I'm guessing that calling Move() is the preferred approach.

::: content/media/fmp4/eme/EMEH264Decoder.cpp
@@ +318,5 @@
>    frame->SetFrameType(sample->is_sync_point ? kGMPKeyFrame : kGMPDeltaFrame);
>    frame->SetBufferType(GMP_BufferLength32);
>  
>    nsTArray<uint8_t> info; // No codec specific per-frame info to pass.
> +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame.release()), false, info, 0);

Be consistent; use Move() instead of release() and re-creating a new UniquePtr.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +288,5 @@
>      gmp_frame_types.AppendElement(ft);
>    }
>  
>    LOGD(("GMP Encode: %llu", (aInputImage->timestamp() * 1000ll)/90));
> +  err = mGMP->Encode(UniquePtr<GMPVideoi420Frame>(Move(frame)),

Just so I understand things, the move has to be explicit right?

@@ +638,5 @@
>    nsTArray<uint8_t> codecSpecificInfo;
>    codecSpecificInfo.AppendElements((uint8_t*)&info, sizeof(GMPCodecSpecificInfo));
>  
>    LOGD(("GMP Decode: %llu, len %d", frame->TimeStamp(), aInputImage._length));
> +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame),

Please be consistent; make |frame| a UniquePtr when it's created, and Move() it when passing ownership here.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8475996 [details] [diff] [review]
convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where appropriate

>+  err = mGMP->Encode(UniquePtr<GMPVideoi420Frame>(Move(frame)),

Would "mGMP->Encode(Move(frame)," work here?

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Chris Pearce (:cpearce) from comment #7)
> Comment on attachment 8475996 [details] [diff] [review]
> convert GMPVideo{Encoder,Decoder} methods to take UniquePtr arguments where
> appropriate
> 
> Review of attachment 8475996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce with nits picked and consistent passing of ownership.
> 
> We should be consistent when using these, and I'm guessing that calling
> Move() is the preferred approach.
> 
> ::: content/media/fmp4/eme/EMEH264Decoder.cpp
> @@ +318,5 @@
> >    frame->SetFrameType(sample->is_sync_point ? kGMPKeyFrame : kGMPDeltaFrame);
> >    frame->SetBufferType(GMP_BufferLength32);
> >  
> >    nsTArray<uint8_t> info; // No codec specific per-frame info to pass.
> > +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame.release()), false, info, 0);
> 
> Be consistent; use Move() instead of release() and re-creating a new
> UniquePtr.

I don't think that works with the upcast required here: frame is of type UniquePtr<GMPVideoEncodedFrameImpl> and we need a UniquePtr<GMPVideoEncodedFrame>.  UniquePtr has a constructor to handle the upcast, but that requires that the deleters associated with each type are implicitly convertable.  Because of needing to write separate deleters for each subtype of GMPVideoFrame, they are not implicitly convertible, and we have to write out things the long way.  (If GMPVideoFrame used virtual destructors, I think the shorter way would work.  I don't completely understand the rationale behind |virtual Destroy()| vs. virtual destructors, since the destructors simply call delete, but I am probably missing something here.)

It's possible I'm wrong; I'll double check.

> ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
> @@ +288,5 @@
> >      gmp_frame_types.AppendElement(ft);
> >    }
> >  
> >    LOGD(("GMP Encode: %llu", (aInputImage->timestamp() * 1000ll)/90));
> > +  err = mGMP->Encode(UniquePtr<GMPVideoi420Frame>(Move(frame)),
> 
> Just so I understand things, the move has to be explicit right?

The Move does have to be explicit, but I don't think (as Karl points out) that the UniquePtr<> constructor must be called; the compiler ought to do that implicitly.  I'll investigate.

> @@ +638,5 @@
> >    nsTArray<uint8_t> codecSpecificInfo;
> >    codecSpecificInfo.AppendElements((uint8_t*)&info, sizeof(GMPCodecSpecificInfo));
> >  
> >    LOGD(("GMP Decode: %llu, len %d", frame->TimeStamp(), aInputImage._length));
> > +  nsresult rv = mGMP->Decode(UniquePtr<GMPVideoEncodedFrame>(frame),
> 
> Please be consistent; make |frame| a UniquePtr when it's created, and Move()
> it when passing ownership here.

Sorry, I missed this case.  I'll fix it up.

Comment 10

[Nathan Froyd [:froydnj]]

This landed: http://hg.mozilla.org/mozilla-central/rev/75a71e95c1a7

I guess mcMerge missed this.