Closed
Bug 1220491
Opened 5 years ago
Closed 5 years ago
clarify ownership relationships for creators of AudioData
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(1 file)
38.46 KB,
patch
|
gerald
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
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.
Attachment #8681719 -
Flags: review?(gsquelart)
Comment 2•5 years ago
|
||
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.
Attachment #8681719 -
Flags: review?(gsquelart) → review+
![]() |
Assignee | |
Comment 3•5 years ago
|
||
(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•5 years ago
|
||
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 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11dd4cfb6563
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 7•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/11dd4cfb6563
status-b2g-v2.5:
--- → fixed
Comment 8•5 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•