Closed Bug 1154499 Opened 5 years ago Closed 4 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
Blocking Flags:

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: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1251714
You need to log in before you can comment on or make changes to this bug.