Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1374253002:
Added functions on libjingle API to start and stop the recording of an RtcEventLog. (Closed)
|
Created:
5 years, 2 months ago by ivoc Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded functions on libjingle API to start and stop the recording of an RtcEventLog.
BUG=webrtc:4741
Committed: https://crrev.com/112a3d81db02d349af0ce6c0827da6d8fbc421a8
Cr-Commit-Position: refs/heads/master@{#10297}
Patch Set 1 : Initial incomplete version. #
Total comments: 1
Patch Set 2 : Added functions to start and stop logging. #
Total comments: 19
Patch Set 3 : Fixed log message and added brackets to if statement. #Patch Set 4 : Rebase and added comment on PeerConnectionFactoryInterface #
Total comments: 7
Patch Set 5 : RtcEventLog takes ownership of the rtc::PlatformFile. #
Total comments: 16
Patch Set 6 : Addressed comments from the sun. #
Total comments: 7
Patch Set 7 : Addressed more comments from the sun. #
Messages
Total messages: 46 (14 generated)
ivoc@webrtc.org changed reviewers: + henrikg@webrtc.org, solenberg@webrtc.org
ivoc@webrtc.org changed required reviewers: + solenberg@webrtc.org
ivoc@webrtc.org changed reviewers: + terelius@webrtc.org
Hi guys, Please have a look at this small CL which adds two functions on the libjingle API to start/stop the recording of an RtcEventLog. To support this, I added a new startLogging function on the RtcEventLog class which logs indefinitely, until the stopLogging function is called.
https://codereview.webrtc.org/1374253002/diff/1/talk/media/webrtc/webrtcvoice... File talk/media/webrtc/webrtcvoiceengine.h (right): https://codereview.webrtc.org/1374253002/diff/1/talk/media/webrtc/webrtcvoice... talk/media/webrtc/webrtcvoiceengine.h:112: bool StartRtcEventLog(rtc::PlatformFile file); So, how do you intend to call this from the rest of the world?
On 2015/09/29 15:03:08, the sun wrote: > https://codereview.webrtc.org/1374253002/diff/1/talk/media/webrtc/webrtcvoice... > File talk/media/webrtc/webrtcvoiceengine.h (right): > > https://codereview.webrtc.org/1374253002/diff/1/talk/media/webrtc/webrtcvoice... > talk/media/webrtc/webrtcvoiceengine.h:112: bool > StartRtcEventLog(rtc::PlatformFile file); > So, how do you intend to call this from the rest of the world? Hmm, after our offline discussion it seems that this CL is incomplete. I will send an update to this CL when I finish the rest.
ivoc@webrtc.org changed reviewers: + glaznev@webrtc.org
ivoc@webrtc.org changed required reviewers: + glaznev@webrtc.org
ivoc@webrtc.org changed reviewers: + tommi@webrtc.org - glaznev@webrtc.org
ivoc@webrtc.org changed required reviewers: + tommi@webrtc.org - glaznev@webrtc.org
Patchset #2 (id:20001) has been deleted
ivoc@webrtc.org changed reviewers: + pthatcher@webrtc.org - tommi@webrtc.org
ivoc@webrtc.org changed required reviewers: + pthatcher@webrtc.org - tommi@webrtc.org
Hi guys, Please have another look at this CL to expose the starting/stopping of the RtcEventLog on the LibJingle/PeerConnection API. Thanks!
Nice work! Some questions below: https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.cc:221: RTC_DCHECK(signaling_thread_->IsCurrent()); Could someone explain what error this check is supposed to capture? https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:83: void StopRtcEventLog() override; I'm not very familiar with the PeerConnection and related code, but I assume that PeerConnectionFactory works like a regular factory which produces PeerConnections with consistent configurations. If this is correct, then I don't understand why we have methods to start and stop logging in the PeerConnectionFactory. Shouldn't it be in PeerConnection and/or PeerConnectionInterface? https://codereview.webrtc.org/1374253002/diff/40001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/40001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.h:49: virtual int StartLogging(FILE* log_file) = 0; Maybe make this function return a bool? What is the project policy regarding indicating success via return codes?
I'm not an owner, and took a high level look. lgtm. https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.cc:221: RTC_DCHECK(signaling_thread_->IsCurrent()); On 2015/10/02 10:07:14, terelius wrote: > Could someone explain what error this check is supposed to capture? It ensures that the function is called on the right thread. (Only allowed to be called on one thread.) https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:82: bool StartRtcEventLog(rtc::PlatformFile file) override; Although not ideal, this is afaik the most reasonable place to have this function. https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1343: LOG(LS_ERROR) << "Could not open AEC dump file stream."; Update text.
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:82: bool StartRtcEventLog(rtc::PlatformFile file) override; On 2015/10/02 10:23:42, Henrik Grunell (webrtc) wrote: > Although not ideal, this is afaik the most reasonable place to have this > function. So is there a reason for having the ChannelManager in the factory rather than in the PeerConnection? Wouldn't it make sense to let the PeerConnection manage its own channels?
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:82: bool StartRtcEventLog(rtc::PlatformFile file) override; On 2015/10/02 12:16:26, terelius wrote: > On 2015/10/02 10:23:42, Henrik Grunell (webrtc) wrote: > > Although not ideal, this is afaik the most reasonable place to have this > > function. > > So is there a reason for having the ChannelManager in the factory rather than in > the PeerConnection? Wouldn't it make sense to let the PeerConnection manage its > own channels? I don't know. I'm not the right person to answer that. :) (It seems to be outside the scope of this CL too.)
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:82: bool StartRtcEventLog(rtc::PlatformFile file) override; On 2015/10/02 12:23:40, Henrik Grunell (webrtc) wrote: > On 2015/10/02 12:16:26, terelius wrote: > > On 2015/10/02 10:23:42, Henrik Grunell (webrtc) wrote: > > > Although not ideal, this is afaik the most reasonable place to have this > > > function. > > > > So is there a reason for having the ChannelManager in the factory rather than > in > > the PeerConnection? Wouldn't it make sense to let the PeerConnection manage > its > > own channels? > > I don't know. I'm not the right person to answer that. :) (It seems to be > outside the scope of this CL too.) Acknowledged.
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:83: void StopRtcEventLog() override; On 2015/10/02 10:07:14, terelius wrote: > I'm not very familiar with the PeerConnection and related code, but I assume > that PeerConnectionFactory works like a regular factory which produces > PeerConnections with consistent configurations. > > If this is correct, then I don't understand why we have methods to start and > stop logging in the PeerConnectionFactory. Shouldn't it be in PeerConnection > and/or PeerConnectionInterface? I think one of the problems is that there can be multiple PeerConnections that correspond to a single VoiceEngine. Also, the start function for AECdump is located here, so it makes sense to keep them together.
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:83: void StopRtcEventLog() override; On 2015/10/05 14:16:32, ivoc wrote: > On 2015/10/02 10:07:14, terelius wrote: > > I'm not very familiar with the PeerConnection and related code, but I assume > > that PeerConnectionFactory works like a regular factory which produces > > PeerConnections with consistent configurations. > > > > If this is correct, then I don't understand why we have methods to start and > > stop logging in the PeerConnectionFactory. Shouldn't it be in PeerConnection > > and/or PeerConnectionInterface? > > I think one of the problems is that there can be multiple PeerConnections that > correspond to a single VoiceEngine. Also, the start function for AECdump is > located here, so it makes sense to keep them together. But why is this logging tied to the voice engine? Is it audio-specific? And do we really want cross-PeerConnection logging? It seems like it would make more sense to log per-Call object and per-PeerConnection. https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1344: if (!rtc::ClosePlatformFile(file)) {}s please https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1349: LOG_RTCERR0(StartLogging); Why does this go through the voice engine? Why doesn't it go straight from the ChannelManager/MediaController to webrtc::Call? It's not audio-specific, is it?
https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnectionfactory.h (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnectionfactory.h:83: void StopRtcEventLog() override; On 2015/10/06 20:49:09, pthatcher1 wrote: > On 2015/10/05 14:16:32, ivoc wrote: > > On 2015/10/02 10:07:14, terelius wrote: > > > I'm not very familiar with the PeerConnection and related code, but I assume > > > that PeerConnectionFactory works like a regular factory which produces > > > PeerConnections with consistent configurations. > > > > > > If this is correct, then I don't understand why we have methods to start and > > > stop logging in the PeerConnectionFactory. Shouldn't it be in PeerConnection > > > and/or PeerConnectionInterface? > > > > I think one of the problems is that there can be multiple PeerConnections that > > correspond to a single VoiceEngine. Also, the start function for AECdump is > > located here, so it makes sense to keep them together. > > But why is this logging tied to the voice engine? Is it audio-specific? And do > we really want cross-PeerConnection logging? It seems like it would make more > sense to log per-Call object and per-PeerConnection. The current place of the logging object is a temporary solution, we have plans to move it to Call when the refactoring of VoE is further along. The reason why VoE is involved is that beside RTP headers we're also logging debug events inside of the audio coding module. I think for now we can accept that logging multiple peerconnections can lead to strange logs (identical SSRCs etc). Since VoE outlives Call, it was easier to put the logging object in VoE, so we don't have to worry about dangling pointers or shared ownership. https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1343: LOG(LS_ERROR) << "Could not open AEC dump file stream."; On 2015/10/02 10:23:42, Henrik Grunell (webrtc) wrote: > Update text. Done. https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1344: if (!rtc::ClosePlatformFile(file)) On 2015/10/06 20:49:09, pthatcher1 wrote: > {}s please Done. https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1349: LOG_RTCERR0(StartLogging); On 2015/10/06 20:49:09, pthatcher1 wrote: > Why does this go through the voice engine? Why doesn't it go straight from the > ChannelManager/MediaController to webrtc::Call? It's not audio-specific, is it? See other reply. For now VoE owns the logging object, but that will change soon.
https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1349: LOG_RTCERR0(StartLogging); On 2015/10/07 14:27:27, ivoc wrote: > On 2015/10/06 20:49:09, pthatcher1 wrote: > > Why does this go through the voice engine? Why doesn't it go straight from > the > > ChannelManager/MediaController to webrtc::Call? It's not audio-specific, is > it? > > See other reply. For now VoE owns the logging object, but that will change soon. Then can you put a comment in PeerConnectionFactory explaining this? - How the method really shouldn't belong there, but it has to because.... - TODO(...): Move this into being PeerConnection => MediaController => webrtc::Call. Or something like that.
https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1349: LOG_RTCERR0(StartLogging); On 2015/10/07 15:59:23, pthatcher1 wrote: > On 2015/10/07 14:27:27, ivoc wrote: > > On 2015/10/06 20:49:09, pthatcher1 wrote: > > > Why does this go through the voice engine? Why doesn't it go straight from > > the > > > ChannelManager/MediaController to webrtc::Call? It's not audio-specific, is > > it? > > > > See other reply. For now VoE owns the logging object, but that will change > soon. > > Then can you put a comment in PeerConnectionFactory explaining this? > - How the method really shouldn't belong there, but it has to because.... > - TODO(...): Move this into being PeerConnection => MediaController => > webrtc::Call. > > Or something like that. Good idea, I added some comments there like you suggested.
lgtm
lgtm
https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1284: if (!rtc::ClosePlatformFile(file)) { This isn't completely clear to me. I assume ClosePlatformFile() is called because ownership of the PlatformFile is effectively transferred to WVoE when this function is called. But below, when StartLogging() is called, you do not call ClosePlatformFile(), and not in StopRtcEventLog() why? https://codereview.webrtc.org/1374253002/diff/80001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:14: #include <limits> where do you use something from limits (in the changes this CL introduces)?
https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1284: if (!rtc::ClosePlatformFile(file)) { On 2015/10/13 11:00:40, the sun wrote: > This isn't completely clear to me. > > I assume ClosePlatformFile() is called because ownership of the PlatformFile is > effectively transferred to WVoE when this function is called. > > But below, when StartLogging() is called, you do not call ClosePlatformFile(), > and not in StopRtcEventLog() why? Hmm, that is actually a really good point. The straightforward answer is that this code is based on the StartAecDump and StopAecDump functions (located just above), which do exactly the same thing. However, that not really a good reason. I see two options for fixing this problem: - We could store the rtc::PlatformFile here in WebRtcVoiceEngine, and call the ClosePlatformFile() whenever StopRtcEventLog() is called. The downside is that it is also possible for the RtcEventLog object to stop logging due to a timeout (there is an internal timer that will stop the log if it goes on for too long), in which case the ClosePlatformFile() function will not be called. - We could pass ownership of the rtc::PlatformFile to the RtcEventLog and make sure the ClosePlatformFile() function is called whenever the log is stopped. This would be a little awkward because only when a rtc::PlatformFile is used to start the log should the ClosePlatformFile() function be called but not in cases it was started using a path or file pointer. I'm leaning toward the second option, do you agree? https://codereview.webrtc.org/1374253002/diff/80001/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log.cc:14: #include <limits> On 2015/10/13 11:00:40, the sun wrote: > where do you use something from limits (in the changes this CL introduces)? Thanks, this looks like a leftover of some earlier code, will remove in the next iteration.
https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1284: if (!rtc::ClosePlatformFile(file)) { On 2015/10/13 13:50:50, ivoc wrote: > On 2015/10/13 11:00:40, the sun wrote: > > This isn't completely clear to me. > > > > I assume ClosePlatformFile() is called because ownership of the PlatformFile > is > > effectively transferred to WVoE when this function is called. > > > > But below, when StartLogging() is called, you do not call ClosePlatformFile(), > > and not in StopRtcEventLog() why? > > Hmm, that is actually a really good point. The straightforward answer is that > this code is based on the StartAecDump and StopAecDump functions (located just > above), which do exactly the same thing. > > However, that not really a good reason. I see two options for fixing this > problem: > - We could store the rtc::PlatformFile here in WebRtcVoiceEngine, and call the > ClosePlatformFile() whenever StopRtcEventLog() is called. The downside is that > it is also possible for the RtcEventLog object to stop logging due to a timeout > (there is an internal timer that will stop the log if it goes on for too long), > in which case the ClosePlatformFile() function will not be called. > - We could pass ownership of the rtc::PlatformFile to the RtcEventLog and make > sure the ClosePlatformFile() function is called whenever the log is stopped. > This would be a little awkward because only when a rtc::PlatformFile is used to > start the log should the ClosePlatformFile() function be called but not in cases > it was started using a path or file pointer. > > I'm leaning toward the second option, do you agree? Do we need any other way to pass the file, or can we always use rtc::PlatformFile?
https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1284: if (!rtc::ClosePlatformFile(file)) { On 2015/10/13 13:50:50, ivoc wrote: > On 2015/10/13 11:00:40, the sun wrote: > > This isn't completely clear to me. > > > > I assume ClosePlatformFile() is called because ownership of the PlatformFile > is > > effectively transferred to WVoE when this function is called. > > > > But below, when StartLogging() is called, you do not call ClosePlatformFile(), > > and not in StopRtcEventLog() why? > > Hmm, that is actually a really good point. The straightforward answer is that > this code is based on the StartAecDump and StopAecDump functions (located just > above), which do exactly the same thing. > > However, that not really a good reason. I see two options for fixing this > problem: > - We could store the rtc::PlatformFile here in WebRtcVoiceEngine, and call the > ClosePlatformFile() whenever StopRtcEventLog() is called. The downside is that > it is also possible for the RtcEventLog object to stop logging due to a timeout > (there is an internal timer that will stop the log if it goes on for too long), > in which case the ClosePlatformFile() function will not be called. > - We could pass ownership of the rtc::PlatformFile to the RtcEventLog and make > sure the ClosePlatformFile() function is called whenever the log is stopped. > This would be a little awkward because only when a rtc::PlatformFile is used to > start the log should the ClosePlatformFile() function be called but not in cases > it was started using a path or file pointer. > > I'm leaning toward the second option, do you agree? Yes, that seems like the best option. Make sure that Chromium expects the ownership to be passed, even in the case of the call failing.
https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvoiceengine.cc:1284: if (!rtc::ClosePlatformFile(file)) { On 2015/10/13 14:29:54, the sun wrote: > On 2015/10/13 13:50:50, ivoc wrote: > > On 2015/10/13 11:00:40, the sun wrote: > > > This isn't completely clear to me. > > > > > > I assume ClosePlatformFile() is called because ownership of the PlatformFile > > is > > > effectively transferred to WVoE when this function is called. > > > > > > But below, when StartLogging() is called, you do not call > ClosePlatformFile(), > > > and not in StopRtcEventLog() why? > > > > Hmm, that is actually a really good point. The straightforward answer is that > > this code is based on the StartAecDump and StopAecDump functions (located just > > above), which do exactly the same thing. > > > > However, that not really a good reason. I see two options for fixing this > > problem: > > - We could store the rtc::PlatformFile here in WebRtcVoiceEngine, and call the > > ClosePlatformFile() whenever StopRtcEventLog() is called. The downside is that > > it is also possible for the RtcEventLog object to stop logging due to a > timeout > > (there is an internal timer that will stop the log if it goes on for too > long), > > in which case the ClosePlatformFile() function will not be called. > > - We could pass ownership of the rtc::PlatformFile to the RtcEventLog and make > > sure the ClosePlatformFile() function is called whenever the log is stopped. > > This would be a little awkward because only when a rtc::PlatformFile is used > to > > start the log should the ClosePlatformFile() function be called but not in > cases > > it was started using a path or file pointer. > > > > I'm leaning toward the second option, do you agree? > > Yes, that seems like the best option. Make sure that Chromium expects the > ownership to be passed, even in the case of the call failing. @terelius: Possibly, but I don't know if using a rtc::PlatformFile would be convenient for other applications than Chromium. We can remove the other options later if they turn out to be useless. @the sun: I will keep that in mind when I work on the Chromium part :)
I made the changes based on the review comments from solenberg@, now RtcEventLog takes ownership of the rtc::PlatformFile, to ensure that ClosePlatformFile() is always called.
https://codereview.webrtc.org/1374253002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1283: if (!rtc::ClosePlatformFile(file)) { RtcEventLogImpl::StartLogging() takes ownership of the platform file (it deletes it if the function fails), so this will likely always spit out the below warning, for no reason. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:157: platform_file_(static_cast<rtc::PlatformFile>(0)), Use C++11 initialization (at member declaration). Also, initialize to rtc::kInvalidPlatformFileValue. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:180: int RtcEventLogImpl::StartLogging(FILE* log_file) { Used? https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:199: if (currently_logging_) please, always use braces https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:201: RTC_DCHECK(!using_platform_file) https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:398: rtc::ClosePlatformFile(platform_file_); platform_file_ is never set. Also, you don't need using_platform_file_; you can compare with rtc::kInvalidPlatformFileValue. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:50: virtual int StartLogging(FILE* log_file) = 0; Can this be removed? Together with the stdio.h include? https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:55: virtual int StartLogging(rtc::PlatformFile& log_file) = 0; Why doesn't this function return a bool?
https://codereview.webrtc.org/1374253002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1283: if (!rtc::ClosePlatformFile(file)) { On 2015/10/14 09:29:09, the sun wrote: > RtcEventLogImpl::StartLogging() takes ownership of the platform file (it deletes > it if the function fails), so this will likely always spit out the below > warning, for no reason. Good point, removed. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:157: platform_file_(static_cast<rtc::PlatformFile>(0)), On 2015/10/14 09:29:09, the sun wrote: > Use C++11 initialization (at member declaration). Also, initialize to > rtc::kInvalidPlatformFileValue. Nice, was not familiar with this. I changed it for the other variables as well and got rid of the constructor. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:180: int RtcEventLogImpl::StartLogging(FILE* log_file) { On 2015/10/14 09:29:09, the sun wrote: > Used? Not currently, I left it in because I thought it might come in handy, but I guess it's better to wait until it's actually needed, so I removed it. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:199: if (currently_logging_) On 2015/10/14 09:29:09, the sun wrote: > please, always use braces Done. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:201: On 2015/10/14 09:29:09, the sun wrote: > RTC_DCHECK(!using_platform_file) Done. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.cc:398: rtc::ClosePlatformFile(platform_file_); On 2015/10/14 09:29:09, the sun wrote: > platform_file_ is never set. Also, you don't need using_platform_file_; you can > compare with rtc::kInvalidPlatformFileValue. Good points, have updated. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:50: virtual int StartLogging(FILE* log_file) = 0; On 2015/10/14 09:29:09, the sun wrote: > Can this be removed? Together with the stdio.h include? Done and Done. https://codereview.webrtc.org/1374253002/diff/100001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:55: virtual int StartLogging(rtc::PlatformFile& log_file) = 0; On 2015/10/14 09:29:09, the sun wrote: > Why doesn't this function return a bool? I don't remember the reason, but I agree that bool is nicer here. Changed.
https://codereview.webrtc.org/1374253002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1281: if (!voe_wrapper_->codec()->GetEventLog()->StartLogging(file)) { I think you should just return voe_wrapper_->codec()->GetEventLog()->StartLogging(file); https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:48: virtual bool StartLogging(rtc::PlatformFile& log_file) = 0; Send this in by value instead. Non-const references are banned btw (use pointers instead, for mutable parameters). https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:48: virtual bool StartLogging(rtc::PlatformFile& log_file) = 0; Are we lacking a unit test for this function?
https://codereview.webrtc.org/1374253002/diff/120001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1374253002/diff/120001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvoiceengine.cc:1281: if (!voe_wrapper_->codec()->GetEventLog()->StartLogging(file)) { On 2015/10/14 13:36:15, the sun wrote: > I think you should just > return voe_wrapper_->codec()->GetEventLog()->StartLogging(file); Okay, sounds good. https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:48: virtual bool StartLogging(rtc::PlatformFile& log_file) = 0; On 2015/10/14 13:36:16, the sun wrote: > Send this in by value instead. Non-const references are banned btw (use pointers > instead, for mutable parameters). Done. https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:48: virtual bool StartLogging(rtc::PlatformFile& log_file) = 0; On 2015/10/14 13:36:16, the sun wrote: > Are we lacking a unit test for this function? There is indeed no unit test for this one, however the other StartLogging function is tested pretty extensively (and is very similar to this one). I looked into this a little further, and it seems not trivial to add a unit test for this one, as there seems to be no way to actually open an rtc::Platformfile in webrtc/base/platform_file.h. Adding a function to do that would be possible, but seems a bit outside of the scope of this CL. Do you agree?
lgtm https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1374253002/diff/120001/webrtc/call/rtc_event_lo... webrtc/call/rtc_event_log.h:48: virtual bool StartLogging(rtc::PlatformFile& log_file) = 0; On 2015/10/15 15:36:14, ivoc wrote: > On 2015/10/14 13:36:16, the sun wrote: > > Are we lacking a unit test for this function? > > There is indeed no unit test for this one, however the other StartLogging > function is tested pretty extensively (and is very similar to this one). I > looked into this a little further, and it seems not trivial to add a unit test > for this one, as there seems to be no way to actually open an rtc::Platformfile > in webrtc/base/platform_file.h. Adding a function to do that would be possible, > but seems a bit outside of the scope of this CL. Do you agree? sgtm
The CQ bit was checked by ivoc@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/1374253002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374253002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrikg@webrtc.org, pthatcher@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/1374253002/#ps140001 (title: "Addressed more comments from the sun.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374253002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374253002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/112a3d81db02d349af0ce6c0827da6d8fbc421a8 Cr-Commit-Position: refs/heads/master@{#10297} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
