Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641923003:
Create an Obj-C wrapper of the RtpReceiverObserverInterface. (Closed)
|
Created:
3 years, 11 months ago by Zhi Huang Modified:
3 years, 10 months ago Reviewers:
tkchin_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate an Obj-C wrapper of the RtpReceiverObserverInterface.
Create the RTCRtpReceiverDelegate which is a wrapper over
webrtc::RtpReceiverObserverInterface.
The callback will be called whenever the first rtp packet is received.
Related CL: https://codereview.webrtc.org/2531333003/
BUG=webrtc:6742
Review-Url: https://codereview.webrtc.org/2641923003
Cr-Commit-Position: refs/heads/master@{#16501}
Committed: https://chromium.googlesource.com/external/webrtc/+/4da058c0ddfee9041c04461dc50d15c766a8bcd4
Patch Set 1 : Add Object-C wrapper of the RtpReceiverObserverInterface. #Patch Set 2 : Format #
Total comments: 14
Patch Set 3 : CR comments. #
Total comments: 4
Patch Set 4 : Comments. #
Total comments: 1
Patch Set 5 : CR comments #
Messages
Total messages: 31 (18 generated)
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Create an Obj-C wrapper of the RtpReceiverObserverInterface. Create the RTCRtpReceiverDelegate which is a wrapper over webrtc::RtpReceiverObserverInterface. The callback will be called whenever the first rtp packet is received. Related CL:https://codereview.webrtc.org/2531333003/ BUG=webrtc:6742 ========== to ========== Create an Obj-C wrapper of the RtpReceiverObserverInterface. Create the RTCRtpReceiverDelegate which is a wrapper over webrtc::RtpReceiverObserverInterface. The callback will be called whenever the first rtp packet is received. Related CL: https://codereview.webrtc.org/2531333003/ BUG=webrtc:6742 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
zhihuang@webrtc.org changed reviewers: + tkchin@webrtc.org
Please take a look. Thanks.
just some nits https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h:21: RtpReceiverDelegateAdapter(RTCRtpReceiver *receiver); follow C++ style rules here, not ObjC style rules (for *) https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:24: receiver_ = receiver; assert not nil? https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:30: [[RTCRtpReceiver class] mediaTypeForNativeMediaType:media_type]; don't need class https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:41: BOOL _isObserverRegistered; not used? https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:20: typedef NS_ENUM(NSInteger, RTCMediaType) { nit: RTCRtpMediaType https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:33: * Note: Currently if there are multiple RtpReceivers of the same media type, nit: ObjC files are allowed to have 100 chars per line What does this mean? If I have RtpReceivers A/B/C their delegates get called on multiple threads simultaneously? What thread does this callback occur on? https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:37: didReceiveFirstPacket:(RTCMediaType)mediaType; nit: didReceiveFirstPacketForMediaType:
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Please take another look. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h:21: RtpReceiverDelegateAdapter(RTCRtpReceiver *receiver); On 2017/01/20 02:29:09, tkchin_webrtc wrote: > follow C++ style rules here, not ObjC style rules (for *) Done. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:24: receiver_ = receiver; On 2017/01/20 02:29:09, tkchin_webrtc wrote: > assert not nil? Done. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:30: [[RTCRtpReceiver class] mediaTypeForNativeMediaType:media_type]; On 2017/01/20 02:29:09, tkchin_webrtc wrote: > don't need class Done. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:41: BOOL _isObserverRegistered; On 2017/01/20 02:29:09, tkchin_webrtc wrote: > not used? I'll just remove this. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h (right): https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:20: typedef NS_ENUM(NSInteger, RTCMediaType) { On 2017/01/20 02:29:09, tkchin_webrtc wrote: > nit: RTCRtpMediaType Done. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:33: * Note: Currently if there are multiple RtpReceivers of the same media type, On 2017/01/20 02:29:09, tkchin_webrtc wrote: > nit: ObjC files are allowed to have 100 chars per line > > What does this mean? If I have RtpReceivers A/B/C their delegates get called on > multiple threads simultaneously? What thread does this callback occur on? Take the audio receivers for example. In the native code, all the audio receivers share a audio channel and they all listen to the same signal (SignalFirstPacketReceived) from audio channel. Once the first packet is received by the audio channel, the signal will be fired, and all the receiver will be notified. Same thing for video receivers. https://cs.chromium.org/chromium/src/third_party/webrtc/api/rtpreceiverinterf... We plan to improve this in the future. https://codereview.webrtc.org/2641923003/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:37: didReceiveFirstPacket:(RTCMediaType)mediaType; On 2017/01/20 02:29:09, tkchin_webrtc wrote: > nit: didReceiveFirstPacketForMediaType: Done.
> On 2017/01/20 02:29:09, tkchin_webrtc wrote: > > nit: ObjC files are allowed to have 100 chars per line > > > > What does this mean? If I have RtpReceivers A/B/C their delegates get called > on > > multiple threads simultaneously? What thread does this callback occur on? > > Take the audio receivers for example. In the native code, all the audio > receivers share a audio channel and they all listen to the same signal > (SignalFirstPacketReceived) from audio channel. Once the first packet is > received by the audio channel, the signal will be fired, and all the receiver > will be notified. > > Same thing for video receivers. Can you clarify the comment please, providing an example? Also, please add what queue the callback will be called on.
On 2017/01/20 23:46:25, tkchin_webrtc wrote: > > On 2017/01/20 02:29:09, tkchin_webrtc wrote: > > > nit: ObjC files are allowed to have 100 chars per line > > > > > > What does this mean? If I have RtpReceivers A/B/C their delegates get called > > on > > > multiple threads simultaneously? What thread does this callback occur on? > > > > Take the audio receivers for example. In the native code, all the audio > > receivers share a audio channel and they all listen to the same signal > > (SignalFirstPacketReceived) from audio channel. Once the first packet is > > received by the audio channel, the signal will be fired, and all the receiver > > will be notified. > > > > Same thing for video receivers. > > Can you clarify the comment please, providing an example? > Also, please add what queue the callback will be called on. I have updated the comment. Now forgive my ignorance, but I don't understand the question "what queue the callback will be called on".
On 2017/01/24 02:18:15, Zhi Huang wrote: > On 2017/01/20 23:46:25, tkchin_webrtc wrote: > > > On 2017/01/20 02:29:09, tkchin_webrtc wrote: > > > > nit: ObjC files are allowed to have 100 chars per line > > > > > > > > What does this mean? If I have RtpReceivers A/B/C their delegates get > called > > > on > > > > multiple threads simultaneously? What thread does this callback occur on? > > > > > > Take the audio receivers for example. In the native code, all the audio > > > receivers share a audio channel and they all listen to the same signal > > > (SignalFirstPacketReceived) from audio channel. Once the first packet is > > > received by the audio channel, the signal will be fired, and all the > receiver > > > will be notified. > > > > > > Same thing for video receivers. > > > > Can you clarify the comment please, providing an example? > > Also, please add what queue the callback will be called on. > > I have updated the comment. > Now forgive my ignorance, but I don't understand the question "what queue the > callback will be called on". Ping. Any comments on this? Thanks.
lgtm https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h (right): https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver+Private.h:26: __weak RTCRtpReceiver *receiver_; * https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm (right): https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:24: ASSERT(receiver); Use ObjC style assert e.g. NSParameterAssert or RTC_CHECK. https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:34: didReceiveFirstPacketForMediaType:packet_media_type]; doesn't fit on same line? Align colons if so https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h (right): https://codereview.webrtc.org/2641923003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCRtpReceiver.h:63: /** The delegate for this RtpReceiver. */ nit: remove extra space before RtpReceiver https://codereview.webrtc.org/2641923003/diff/140001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm (right): https://codereview.webrtc.org/2641923003/diff/140001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCRtpReceiver.mm:34: didReceiveFirstPacketForMediaType:packet_media_type]; nit: add two more spaces for indent
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2641923003/#ps160001 (title: "CR comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/4922)
The CQ bit was checked by zhihuang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/4928)
The CQ bit was checked by zhihuang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486579093960410,
"parent_rev": "bb46b95dbea0857651c32883efc02bbac0f90c09", "commit_rev":
"4da058c0ddfee9041c04461dc50d15c766a8bcd4"}
Message was sent while issue was closed.
Description was changed from ========== Create an Obj-C wrapper of the RtpReceiverObserverInterface. Create the RTCRtpReceiverDelegate which is a wrapper over webrtc::RtpReceiverObserverInterface. The callback will be called whenever the first rtp packet is received. Related CL: https://codereview.webrtc.org/2531333003/ BUG=webrtc:6742 ========== to ========== Create an Obj-C wrapper of the RtpReceiverObserverInterface. Create the RTCRtpReceiverDelegate which is a wrapper over webrtc::RtpReceiverObserverInterface. The callback will be called whenever the first rtp packet is received. Related CL: https://codereview.webrtc.org/2531333003/ BUG=webrtc:6742 Review-Url: https://codereview.webrtc.org/2641923003 Cr-Commit-Position: refs/heads/master@{#16501} Committed: https://chromium.googlesource.com/external/webrtc/+/4da058c0ddfee9041c04461dc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/4da058c0ddfee9041c04461dc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
