Closed Bug 1154499 Opened 10 years ago Closed 9 years ago

Move away from ScopedDeletePtr for MediaPipeline

Categories

(Core :: WebRTC: Signaling, defect, P4)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1251714
Tracking Status
firefox40 --- affected

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file, 1 obsolete file)

We might also look at moving off nsAutoPtr too.
I realize that this patch is a little lame in that there are a lot of calls to .get(). But I don't see an easy fix without actually making some more changes to SrtpFlow and other APIs. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d45615407f18
Attached file MozReview Request: bz://1154499/mt (obsolete) —
/r/7053 - Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries Pull down this commit: hg pull -r 649a9c6f30b0fd51837f28214ce0fe33c05ce2c7 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592546 - Flags: review?(ekr)
Off-topic but I landed https://hg.mozilla.org/mozilla-central/rev/570bb53a3e7b on m-c with a typo in the bug number. Anybody who comes here looking for info should be redirected to bug 1154459 instead. Sorry!
Comment on attachment 8592546 [details] MozReview Request: bz://1154499/mt https://reviewboard.mozilla.org/r/7051/#review7757 There's a lot of repetition here. Should we make a helper function to take an unsigned char\* and size_t and return a UniquePtr<unsigned char[]> with the contents? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:487 (Diff revision 1) > - inner_data[3]); > + inner_data.get()[3]); UniquePtr appears to implement operator[]. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:478 (Diff revision 1) > - nsresult res = rtp_.recv_srtp_->UnprotectRtp(inner_data, > + nsresult res = rtp_.recv_srtp_->UnprotectRtp(inner_data.get(), On the principle of "never unbox" perhaps we should instead make these take a UniquePtr<unsigned char[]>* ?
Attachment #8592546 - Flags: review?(ekr)
Comment on attachment 8592546 [details] MozReview Request: bz://1154499/mt https://reviewboard.mozilla.org/r/7051/#review7783 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:487 (Diff revision 1) > - inner_data[3]); > + inner_data.get()[3]); Fix this.
Attachment #8592546 - Flags: review+
Comment on attachment 8592546 [details] MozReview Request: bz://1154499/mt /r/7053 - Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries, r=ekr Pull down this commit: hg pull -r 7b8713430b200bff148a7748fb0b5161c5b0b92a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592546 - Flags: review+
Comment on attachment 8592546 [details] MozReview Request: bz://1154499/mt Here you go.
Attachment #8592546 - Flags: review?(ekr)
Comment on attachment 8592546 [details] MozReview Request: bz://1154499/mt /r/7053 - Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries, r=ekr Pull down this commit: hg pull -r 8a512953ead8f09b36b7e75a0d39bf49cbae3112 https://reviewboard-hg.mozilla.org/gecko/
I screwed up the last one, sorry. Can you double check that I'm not crazy.
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Attachment #8592546 - Attachment is obsolete: true
Attachment #8592546 - Flags: review?(ekr)
Attachment #8620042 - Flags: review?(ekr)
Comment on attachment 8620042 [details] MozReview Request: Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries, r=ekr Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries, r=ekr
Looks like Randell's buffer copying reduction fix landed, which rotted this.
Assignee: nobody → martin.thomson
https://reviewboard.mozilla.org/r/7051/#review7785 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:855 (Diff revisions 1 - 2) > void MediaPipelineTransmit::PipelineListener:: What the heck happened here? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:428 (Diff revisions 3 - 4) > +// This makes a copy of a (const) buffer for passing to SrtpFlow, which modifies > +// buffers in place. The expand argument can be used to ensures that when > +// libstrp writes into the same buffer with the answer, it has enough room. There is no expand argument ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:430 (Diff revisions 3 - 4) > +// libstrp writes into the same buffer with the answer, it has enough room. 1. libsrtp 2. There is no expand argument ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:754 (Diff revisions 3 - 4) > nsAutoPtr<DataBuffer> buf(new DataBuffer(static_cast<const uint8_t *>(data), > len, len + SRTP_MAX_EXPANSION)); This is still an nsAutopPtr I'm kind of confused about your intentions for this patch. It seems like you didn't totally convert to UniquePtr<>?
Attachment #8620042 - Flags: review?(ekr) → review-
Looks like this was fixed as part of Bug 1251714.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: