Closed
Bug 1154499
Opened 10 years ago
Closed 9 years ago
Move away from ScopedDeletePtr for MediaPipeline
Categories
(Core :: WebRTC: Signaling, defect, P4)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
DUPLICATE
of bug 1251714
Tracking | Status | |
---|---|---|
firefox40 | --- | affected |
backlog | webrtc/webaudio+ |
People
(Reporter: mt, Assigned: mt)
Details
Attachments
(1 file, 1 obsolete file)
We might also look at moving off nsAutoPtr too.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
/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)
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8592546 [details]
MozReview Request: bz://1154499/mt
Here you go.
Attachment #8592546 -
Flags: review?(ekr)
Assignee | ||
Comment 8•10 years ago
|
||
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/
Assignee | ||
Comment 9•10 years ago
|
||
I screwed up the last one, sorry. Can you double check that I'm not crazy.
Updated•10 years ago
|
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8592546 -
Attachment is obsolete: true
Attachment #8592546 -
Flags: review?(ekr)
Attachment #8620042 -
Flags: review?(ekr)
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Comment 13•10 years ago
|
||
Looks like Randell's buffer copying reduction fix landed, which rotted this.
Assignee: nobody → martin.thomson
Comment 14•10 years ago
|
||
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<>?
Updated•10 years ago
|
Attachment #8620042 -
Flags: review?(ekr) → review-
![]() |
||
Comment 15•9 years ago
|
||
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.
Description
•