1154499 - Move away from ScopedDeletePtr for MediaPipeline

Status: RESOLVED DUPLICATE of bug 1251714

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

Description

[Martin Thomson [:mt:]]

We might also look at moving off nsAutoPtr too.

Comment 1

[Martin Thomson [:mt:]]

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

Comment 2

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://1154499/mt

/r/7053 - Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries

Pull down this commit:

hg pull -r 649a9c6f30b0fd51837f28214ce0fe33c05ce2c7 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

[Eric Rescorla (:ekr)]

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[]>* ?

Comment 5

[Martin Thomson [:mt:]]

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.

Comment 6

[Martin Thomson [:mt:]]

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/

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8592546 [details]
MozReview Request: bz://1154499/mt

Here you go.

Comment 8

[Martin Thomson [:mt:]]

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/

Comment 9

[Martin Thomson [:mt:]]

I screwed up the last one, sorry.  Can you double check that I'm not crazy.

Comment 11

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1154499 - Move MediaPipeline to UniquePtr for temporaries, r=ekr

Comment 12

[Martin Thomson [:mt:]]

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

Comment 13

[Martin Thomson [:mt:]]

Looks like Randell's buffer copying reduction fix landed, which rotted this.

Comment 14

[Eric Rescorla (:ekr)]

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<>?

Comment 15

[:Cykesiopka]

Looks like this was fixed as part of Bug 1251714.