1220491 - clarify ownership relationships for creators of AudioData

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Nathan Froyd [:froydnj]]

The way we pass in AudioDataValue arrays into AudioData is non-uniform:
sometimes we have nsAutoArrayPtrs, sometimes we don't, and it's not
immediately obvious from the function signature of the constructor that
we're actually taking ownership of this array.  Let's fix that by using
UniquePtr<AudioDataValue[]> smart pointers to hold the data prior to
creating AudioData values, and for passing in to AudioData's
constructor.  Using standard-er C++ things instead of our homegrown ones
is a good thing.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: clarify ownership relationships for creators of AudioData

Not exactly sure who should review this, so trusting that Gerald either has the
authority to review, or that he has the good cheer to pass it off on somebody
who does have the authority.

Comment 2

[Gerald Squelart [:gerald] (he/him)]

Comment on attachment 8681719 [details] [diff] [review]
clarify ownership relationships for creators of AudioData

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

I've checked with cpearce (module owner). He's happy with the changes in principle, and for me to do the in-depth review.

r+ with bikeshedding bait.

::: dom/media/MediaData.h
@@ +123,5 @@
>    AudioData(int64_t aOffset,
>              int64_t aTime,
>              int64_t aDuration,
>              uint32_t aFrames,
> +            UniquePtr<AudioDataValue[]> aData,

I initially wrote: "You should accept aData as r-value reference, to avoid an unnecessary creation of a local UniquePtr."

But then I read UniquePtr's documentation (imagine that!), which advises: "To unconditionally transfer ownership of a UniquePtr into a method, use a |UniquePtr| argument.  To conditionally transfer ownership of a resource into a method, should the method want it, use a |UniquePtr&&| argument."
So I guess I should defer to the docs; but has this been discussed already? (Since the stdlib prefers unique_ptr&& everywhere.)

Also it's probably an insignificant cost compared to the rest of the audio processing.
So I'll leave the final decision to you.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Gerald Squelart [:gerald] from comment #2)
> ::: dom/media/MediaData.h
> @@ +123,5 @@
> >    AudioData(int64_t aOffset,
> >              int64_t aTime,
> >              int64_t aDuration,
> >              uint32_t aFrames,
> > +            UniquePtr<AudioDataValue[]> aData,
> 
> I initially wrote: "You should accept aData as r-value reference, to avoid
> an unnecessary creation of a local UniquePtr."
> 
> But then I read UniquePtr's documentation (imagine that!), which advises:
> "To unconditionally transfer ownership of a UniquePtr into a method, use a
> |UniquePtr| argument.  To conditionally transfer ownership of a resource
> into a method, should the method want it, use a |UniquePtr&&| argument."
> So I guess I should defer to the docs; but has this been discussed already?
> (Since the stdlib prefers unique_ptr&& everywhere.)

I don't know that we've had a discussion about this.  A quick grep suggests that |UniquePtr| has a small mindshare lead over UniquePtr&&.  The recently released C++ Core Guidelines suggest passing arguments using values:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-uniqueptrparam

It doesn't really explain why this would be preferable to rvalue reference-passing.  I think value arguments are slightly faster (since the callee doesn't have to look through the reference?), while rvalue arguments are more economical with codesize (since you aren't required to destruct the temporary value?), but I'd have to whip up some examples to verify that.

Comment 4

[Karl Tomlinson (:karlt)]

IMO clarity of transfer of ownership trumps trivial construction/destruction costs, which I suspect probably close to even out with dereferencing and caller destruction costs, after optimization.  i.e. I'd agree with the docs.

https://groups.google.com/forum/#!msg/mozilla.dev.platform/Ji3T5kipJzA/4GIk9BEtjsoJ

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11dd4cfb6563

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11dd4cfb6563

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/11dd4cfb6563

Comment 8

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion