Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052233002:
Enable passing receive stream ID to the decoder factory. (Closed)
|
Created:
4 years, 6 months ago by sakal Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters.
VideoDecoderParams contains the id of the receive video
stream. Motivation behind this change is to enable down
stream apps easier map raw non-decoded data to incoming
streams.
BUG=b/28636393
Committed: https://crrev.com/1fd959593677a3b2978c488bd796e966f8025b42
Cr-Commit-Position: refs/heads/master@{#13250}
Patch Set 1 #Patch Set 2 : Formatting #
Total comments: 5
Patch Set 3 : Remove ssrc method from VideoDecoder interface for now #Patch Set 4 : Changes according to perkj's comments #
Total comments: 9
Patch Set 5 : pthatcher1's solution implemented #
Total comments: 9
Patch Set 6 : Changes according to pthatcher1's comments #
Total comments: 8
Patch Set 7 : Changes according to perkj's comments #2 && git cl format #
Total comments: 3
Patch Set 8 : Changes according to pthatcher1's comments #2 #Patch Set 9 : Add tests. #
Total comments: 1
Patch Set 10 : Nit fixes. #
Messages
Total messages: 45 (16 generated)
Description was changed from ========== Enable passing SSRC to the decoder factory. This acts a temporary solution for some use cases before a way to attach sinks that consume encoded data to tracks is implemented. ========== to ========== Enable passing SSRC to the decoder factory. This acts a temporary solution for some use cases before a way to attach sinks that consume encoded data to tracks is implemented. ==========
sakal@webrtc.org changed reviewers: + perkj@webrtc.org
Per, take a look please.
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2052233002/20001
https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h File webrtc/video_decoder.h (right): https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h#ne... webrtc/video_decoder.h:87: virtual uint32_t ssrc() const { return 0; } please remove from the interface.
Description was changed from ========== Enable passing SSRC to the decoder factory. This acts a temporary solution for some use cases before a way to attach sinks that consume encoded data to tracks is implemented. ========== to ========== Enable passing SSRC to the decoder factory. This acts a temporary solution for some use cases before a way to attach sinks that consume encoded data to tracks is implemented. BUG=b/28636393 ==========
perkj@webrtc.org changed reviewers: + noahric@chromium.org, pthatcher@webrtc.org
Peter, Noah, can you please take a look? Is this what you need? https://codereview.webrtc.org/2052233002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:27: virtual webrtc::VideoDecoder* CreateVideoDecoderForSsrc( nit: Can you move this to below the existing CreateVideoDecoder and remove the comments. They don't add any value.
https://codereview.webrtc.org/2052233002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:27: virtual webrtc::VideoDecoder* CreateVideoDecoderForSsrc( On 2016/06/10 13:46:39, perkj_webrtc wrote: > nit: Can you move this to below the existing CreateVideoDecoder and remove the > comments. They don't add any value. Done. https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h File webrtc/video_decoder.h (right): https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h#ne... webrtc/video_decoder.h:87: virtual uint32_t ssrc() const { return 0; } On 2016/06/10 13:22:20, perkj_webrtc wrote: > please remove from the interface. Done.
https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h File webrtc/video_decoder.h (right): https://codereview.webrtc.org/2052233002/diff/20001/webrtc/video_decoder.h#ne... webrtc/video_decoder.h:87: virtual uint32_t ssrc() const { return 0; } On 2016/06/14 13:28:44, sakal wrote: > On 2016/06/10 13:22:20, perkj_webrtc wrote: > > please remove from the interface. > > Done. See my comment elsewhere; if receive streams can have multiple ssrcs, then you need to keep this so the allocate-or-reuse can make sure to keep the ssrcs consistent. (Or if the ssrc can change through an allocate-or-reuse point) https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2262: AllocatedDecoder decoder = (*old_decoders)[i]; Can receive streams have multiple receive ssrcs? Or is "stream" here synonymous with single-ssrc? (If it is, it feels like it shouldn't need to be passed in, and there should be a field for it on the receive stream)
https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:29: virtual webrtc::VideoDecoder* CreateVideoDecoderForSsrc( This doesn't seem right to me. Why would a video decoder need to know about SSRCs? SSRCs are an RTP/transport thing, and decoders really just take an EncodedImage and give a VideoFrame. That don't know anything about how the frame was packetized, transmitted, etc. If you need some kind of metadata about the frames that will be decoded, it should be abstracted away from being and SSRC. https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:30: uint32_t ssrc, And if we do pass in metadata about the frames that will be decoded, I think we should have one method (CreateVideoDecoder) that gets a struct of metadata, and not two methods. And the metadata should be the last argument, not the first. Since CreateVideoDecoder is only called in one place and implemented in one place, it shouldn't be that hard to add an argument. https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2262: AllocatedDecoder decoder = (*old_decoders)[i]; On 2016/06/14 17:51:10, noahric wrote: > Can receive streams have multiple receive ssrcs? Or is "stream" here synonymous > with single-ssrc? > > (If it is, it feels like it shouldn't need to be passed in, and there should be > a field for it on the receive stream) There is more than one. See below how there is a list of ssrcs_, and this is just getting the first one.
https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:29: virtual webrtc::VideoDecoder* CreateVideoDecoderForSsrc( On 2016/06/14 18:06:32, pthatcher1 wrote: > This doesn't seem right to me. Why would a video decoder need to know about > SSRCs? SSRCs are an RTP/transport thing, and decoders really just take an > EncodedImage and give a VideoFrame. That don't know anything about how the > frame was packetized, transmitted, etc. That's specifically the problem on android. The optimal way to decode video on android MediaCodec decoders is for them to be directly given the rendering surface (in the app, if possible) to render to. Having intermediate buffers or textures increases latency, battery usage, and heat. Feel free to stop by if you want some more background on it. I think there are cleaner long term plans, but this is a short term unblocker of some better rendering on android. > > If you need some kind of metadata about the frames that will be decoded, it > should be abstracted away from being and SSRC. We could do that, provided that we can connect the dots between the renderer and the decoder. Roughly speaking, on Android, the decoder wants to consume information that the renderer produces (the surface). ssrcs are just the simplest way of doing that, since they identify the stream. And to be clear, it's not about frames. It's about the stream, or at least the renderer/decoder relationship. https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:30: uint32_t ssrc, On 2016/06/14 18:06:32, pthatcher1 wrote: > And if we do pass in metadata about the frames that will be decoded, I think we > should have one method (CreateVideoDecoder) that gets a struct of metadata, and > not two methods. And the metadata should be the last argument, not the first. > Since CreateVideoDecoder is only called in one place and implemented in one > place, it shouldn't be that hard to add an argument. There isn't only one implementation. This is a publicly implementable API, so you can't just add an argument. As for it being a struct, it could be a struct that just contains an ssrc now, if that's preferable. Or it could be some opaque identifier, as long as renderers can use the same identifier.
https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:29: virtual webrtc::VideoDecoder* CreateVideoDecoderForSsrc( On 2016/06/14 18:26:21, noahric wrote: > On 2016/06/14 18:06:32, pthatcher1 wrote: > > This doesn't seem right to me. Why would a video decoder need to know about > > SSRCs? SSRCs are an RTP/transport thing, and decoders really just take an > > EncodedImage and give a VideoFrame. That don't know anything about how the > > frame was packetized, transmitted, etc. > That's specifically the problem on android. The optimal way to decode video on > android MediaCodec decoders is for them to be directly given the rendering > surface (in the app, if possible) to render to. Having intermediate buffers or > textures increases latency, battery usage, and heat. > > Feel free to stop by if you want some more background on it. > > I think there are cleaner long term plans, but this is a short term unblocker of > some better rendering on android. I still don't see why it needs to know SSRCs. This whole thing seems very hacky. Perhaps will have to chat in person to understand what you're trying to do. > > > > > If you need some kind of metadata about the frames that will be decoded, it > > should be abstracted away from being and SSRC. > We could do that, provided that we can connect the dots between the renderer and > the decoder. Roughly speaking, on Android, the decoder wants to consume > information that the renderer produces (the surface). ssrcs are just the > simplest way of doing that, since they identify the stream. Using SSRCs as stream identifiers is very ugly thing that we're trying to remove from the code base, not expand. The more I think about it, the more I think that if you need metadata about an EncodedImage, (because you're trying to hack a decoder into looking like an EncodedImage sink), then I think it makes the most sense to put that metadata on the EncodedImage. That might be generally useful anyway. > > And to be clear, it's not about frames. It's about the stream, or at least the > renderer/decoder relationship. > https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:30: uint32_t ssrc, On 2016/06/14 18:26:21, noahric wrote: > On 2016/06/14 18:06:32, pthatcher1 wrote: > > And if we do pass in metadata about the frames that will be decoded, I think > we > > should have one method (CreateVideoDecoder) that gets a struct of metadata, > and > > not two methods. And the metadata should be the last argument, not the first. > > > Since CreateVideoDecoder is only called in one place and implemented in one > > place, it shouldn't be that hard to add an argument. > > There isn't only one implementation. This is a publicly implementable API, so > you can't just add an argument. Sure you can. You might have to have a little transition period where you have two methods with the same name and different args, but long-term target having one method. We do this all the time. But I agree that it would be nicer to not change this method and instead add the metadata elsewhere (such as on the EncodedImage) > > As for it being a struct, it could be a struct that just contains an ssrc now, > if that's preferable. Or it could be some opaque identifier, as long as > renderers can use the same identifier. We don't really have renderers any more. We have VideoFrame sinks. But regardless of what we call them, all they get are VideoFrames, and those don't have SSRCs either. So I don't see why you would want to use SSRCs to try and correlate EncodedImages to VideoFrames. When would a renderer/sink be given an SSRC? That doesn't make sense.
https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2271: external_decoder_factory_->CreateVideoDecoderForSsrc(ssrc, type); Here's a possible solution: 1. The WebRtcVideoChannel2::WebRtcVideoReceiveStream is passed an ID, which is the same as the RtpReceiver's id() (and in the future, MID). But the WebRtcVideoReceiveStream doesn't care. It's just an ID. 2. That id() is passed into CreateVideoDecoder here as CreateVideoDecoder(type, {id()}), where hopefully the last thing is a struct with a .receive_stream_id. 3. We expose the SSRCs in the RtpReceiver::GetParameters. 4. By enumerating over the PeerConnection's RtpReceivers, you can get the mapping of ssrc <-> stream_id and thus the decoder can know the ssrc from the .stream_id. Just an idea, but it wouldn't be very hard, and it wouldn't be too hacky.
On 2016/06/14 21:12:04, pthatcher1 wrote: > https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:2271: > external_decoder_factory_->CreateVideoDecoderForSsrc(ssrc, type); > Here's a possible solution: > > 1. The WebRtcVideoChannel2::WebRtcVideoReceiveStream is passed an ID, which is > the same as the RtpReceiver's id() (and in the future, MID). But the > WebRtcVideoReceiveStream doesn't care. It's just an ID. > > 2. That id() is passed into CreateVideoDecoder here as CreateVideoDecoder(type, > {id()}), where hopefully the last thing is a struct with a .receive_stream_id. > > 3. We expose the SSRCs in the RtpReceiver::GetParameters. > > 4. By enumerating over the PeerConnection's RtpReceivers, you can get the > mapping of ssrc <-> stream_id and thus the decoder can know the ssrc from the > .stream_id. > > Just an idea, but it wouldn't be very hard, and it wouldn't be too hacky. Whatever works for the time being. There are multiple projects that want to do something similar on both the send side and receive side. In the long run, I would like to have some kind of VideoTrackSource that can produce encoded data. And a VideoTrackRenderer that accept encoded data. In the shorder run, if the RtpReceiver id works for noah just as well as SSRCs then we can look into that.
On 2016/06/14 21:12:04, pthatcher1 wrote: > https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2052233002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:2271: > external_decoder_factory_->CreateVideoDecoderForSsrc(ssrc, type); > Here's a possible solution: > > 1. The WebRtcVideoChannel2::WebRtcVideoReceiveStream is passed an ID, which is > the same as the RtpReceiver's id() (and in the future, MID). But the > WebRtcVideoReceiveStream doesn't care. It's just an ID. > > 2. That id() is passed into CreateVideoDecoder here as CreateVideoDecoder(type, > {id()}), where hopefully the last thing is a struct with a .receive_stream_id. > > 3. We expose the SSRCs in the RtpReceiver::GetParameters. > > 4. By enumerating over the PeerConnection's RtpReceivers, you can get the > mapping of ssrc <-> stream_id and thus the decoder can know the ssrc from the > .stream_id. > > Just an idea, but it wouldn't be very hard, and it wouldn't be too hacky. I've now implemented this patch set #5.
With a little tweaking, I think this will work. https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.... webrtc/api/rtpparameters.h:50: std::vector<uint32_t> ssrcs; This should match the standard here: http://w3c.github.io/webrtc-pc/#idl-def-rtcrtpencodingparameters Which means that RtpEncodingParameters should have one ssrc. And RtpParameters should have any SSRCs, only through the RtpEncodingParameters. https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideodecoderfactory.h:23: struct WebRtcVideoDecoderFactoryParams { This should be called VideoDecoderParams. https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:922: rtp_params.ssrcs = it->second->GetSsrcs(); This should be: if (!it->second-GetSsrcs().empty()) { rtp_params.encodings[0] = it->second->GetSsrcs()[0]; } https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2210: receive_stream_id_(sp.id), Interesting. That's now what I had in mind originally, but that will probably be easier to use and implement for now, since that comes right from the SessionDescription passed into SetLocalDescription. https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.h:476: const std::string receive_stream_id_; This should just be "id_".
https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.... webrtc/api/rtpparameters.h:50: std::vector<uint32_t> ssrcs; Why is this needed? https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:922: rtp_params.ssrcs = it->second->GetSsrcs(); On 2016/06/15 19:40:00, pthatcher1 wrote: > This should be: > > if (!it->second-GetSsrcs().empty()) { > rtp_params.encodings[0] = it->second->GetSsrcs()[0]; > } Why is this change needed at all ?
https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/api/rtpparameters.... webrtc/api/rtpparameters.h:50: std::vector<uint32_t> ssrcs; On 2016/06/15 20:25:52, perkj_webrtc wrote: > Why is this needed? It's part of the standard. If you want an explanation of why it's in the standard, well, that will take a long time :). Mostly it's useful for apps to know, and in a later version of WebRTC/ORTC, to set. https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:922: rtp_params.ssrcs = it->second->GetSsrcs(); On 2016/06/15 20:25:52, perkj_webrtc wrote: > On 2016/06/15 19:40:00, pthatcher1 wrote: > > This should be: > > > > if (!it->second-GetSsrcs().empty()) { > > rtp_params.encodings[0] = it->second->GetSsrcs()[0]; > > } > > Why is this change needed at all ? Now that you mention it, this part of the change isn't needed at all if sp.id is used as the received_stream_id, because the app can just the ID passed into the SessionDescription to correlate the decoder to the receive stream, even without having the SSRC exposed in the RtpParameters. So, as much as I like a part of the standard implemented, you are correct that it's not needed for the purpose of this CL. As for the purpose: my understanding is that certain applications want to have a hardward-based VideoSinkInterface<EncodedFrame> for certain payload types but not others. We don't have support. So they are hacking around it by making a Decoder act like a VideoSinkInterface<EncodedFrame>. But then they need to correlate a Decoder to an RtpReceiver/ReceiveStream. And do that, they basically need the Decoder to know some ID of the RtpReceiver/ReceiveStream.
Oh, and this needs a new title and some unit tests.
PTYAL. Let's implement the tests once we are settled on the implementation. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideodecoderfactory.h (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideodecoderfactory.h:33: virtual webrtc::VideoDecoder* CreateVideoDecoderWithParams( I had to change the name because otherwise decoders deciding not to implement this would cause warnings which are treated as errors. Having it with the same name broke Android build. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2210: stream_params_(sp), Since we are storing so many variables from StreamParams, might as well store the whole structure? This also give us access to the GetPrimarySsrcs method. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2259: return 0; I am not sure if this should ever happen. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2261: return primary_ssrcs_[0]; I am not sure if receiving side ever has multiple primary SSRCs.
We will have to come up with a unittest as well. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/api/rtpparameters... webrtc/api/rtpparameters.h:27: return active == o.active && max_bitrate_bps == o.max_bitrate_bps; add ssrc comparison.. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2254: std::vector<uint32_t> primary_ssrcs_; nit: This is a local variable and should be named primary_ssrcs
https://codereview.webrtc.org/2052233002/diff/100001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/api/rtpparameters... webrtc/api/rtpparameters.h:27: return active == o.active && max_bitrate_bps == o.max_bitrate_bps; On 2016/06/16 13:28:28, perkj_webrtc wrote: > add ssrc comparison.. Done. https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:2254: std::vector<uint32_t> primary_ssrcs_; On 2016/06/16 13:28:28, perkj_webrtc wrote: > nit: This is a local variable and should be named primary_ssrcs Done.
A few minor issues, mostly that we shouldn't set the SSRC to 0 when it's not set. However, do recall that while this is nice for the implementation of the standard, you'll accomplish what you came to accomplish by just not adding the SSRC to the RtpEncodingParameters. https://codereview.webrtc.org/2052233002/diff/110001/webrtc/api/rtpparameters.h File webrtc/api/rtpparameters.h (right): https://codereview.webrtc.org/2052233002/diff/110001/webrtc/api/rtpparameters... webrtc/api/rtpparameters.h:22: uint32_t ssrc; We could make this an rtc::optional<uint32_t>, since it's possible that it's not set. (We should probably do the same for max_bitrate_bps, but it looks like we'll have to fix that later). https://codereview.webrtc.org/2052233002/diff/110001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2052233002/diff/110001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:922: rtp_params.encodings[0].ssrc = it->second->GetPrimarySsrc(); Either have GetFirstPrimarySsrc() return an rtc::optional or put this in an if block. https://codereview.webrtc.org/2052233002/diff/110001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2052233002/diff/110001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.h:428: uint32_t GetPrimarySsrc() const; It's more like GetFirstPrimarySsrc(), since there is more than one. And since there might not be one, this should either return an rtc::optional or return a bool with an out pointer.
Patchset #8 (id:130001) has been deleted
PTYAL. noahric, can you comment if this looks like a good solution? Does it solve your issue?
lgtm
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2052233002/170001
Description was changed from ========== Enable passing SSRC to the decoder factory. This acts a temporary solution for some use cases before a way to attach sinks that consume encoded data to tracks is implemented. BUG=b/28636393 ========== to ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:170001) has been deleted
Patchset #9 (id:190001) has been deleted
I added some tests. pthatcher1, could you please verify they are well implemented?
Tests look good (with one small nit) https://codereview.webrtc.org/2052233002/diff/210001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2052233002/diff/210001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:842: EXPECT_EQ("FakeStreamParamsId", params[0].receive_stream_id); EXPECT_EQ(sp.id, params[0].receive_stream_id); would be shorter and more robust.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2052233002/#ps230001 (title: "Nit fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2052233002/230001
Description was changed from ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ========== to ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ==========
Message was sent while issue was closed.
Description was changed from ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ========== to ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 ========== to ========== Pass VideoDecoderParams to VideoDecoderFactory and add SSRC to RtpEncodingParameters. VideoDecoderParams contains the id of the receive video stream. Motivation behind this change is to enable down stream apps easier map raw non-decoded data to incoming streams. BUG=b/28636393 Committed: https://crrev.com/1fd959593677a3b2978c488bd796e966f8025b42 Cr-Commit-Position: refs/heads/master@{#13250} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/1fd959593677a3b2978c488bd796e966f8025b42 Cr-Commit-Position: refs/heads/master@{#13250} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
