Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
264793017:
Implements RTP header dumping. (Closed)
|
Created:
6 years, 7 months ago by jiayl Modified:
6 years, 6 months ago Reviewers:
Henrik Grunell, Mallinath (Gone from Chromium), jam, tommi (sloooow) - chröme, vrk (LEFT CHROMIUM) CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplements RTP header dumping.
WebRtcRtpDumpHandler implements RTP header dump creation. It's owned by WebRtcLoggingHandlerHost and receives RTP packet callbacks through P2PSocketHost.
WebRtcRtpDumpWriter is owned by WebRtcDumpHandler and writes the RTP header into a memory buffer. When the in-memory buffer is full, it compresses the data and writes it to the disk.
After the dumping is stopped, WebRtcLoggingHandlerHost calls ReleaseDump to get the completed dump as a .gz file and uploads it.
BUG=363459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273745
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273980
Patch Set 1 #
Total comments: 11
Patch Set 2 : Add SetWebRtcRtpPacketCallback on RenderProcessHost #Patch Set 3 : Add WebRtcRtpDumpWriter #
Total comments: 13
Patch Set 4 : #
Total comments: 43
Patch Set 5 : #
Total comments: 26
Patch Set 6 : #
Total comments: 22
Patch Set 7 : #
Total comments: 8
Patch Set 8 : #
Total comments: 59
Patch Set 9 : for tommi's #
Total comments: 34
Patch Set 10 : Fix threading #
Total comments: 2
Patch Set 11 : #
Total comments: 1
Patch Set 12 : for Henrik's comments #
Total comments: 26
Patch Set 13 : for Henrik's #
Total comments: 20
Patch Set 14 : for vrk's comments #Patch Set 15 : fix std::vector::data() #Patch Set 16 : fix leak #Messages
Total messages: 55 (0 generated)
Hi Jiayang. Some comments and questions. https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS#ne... chrome/browser/media/DEPS:5: "+third_party/libjingle", I'm not so sure we should depend on libjingle here. It's for the RTP dump writer, right? https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:62: virtual void OnIncomingRtpPacket(const uint8* packet, size_t length) OVERRIDE; Who will call these? https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:81: scoped_ptr<talk_base::FifoBuffer> dump_buffer_; Does it have to be a talk_base::FifoBuffer? https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:82: scoped_ptr<cricket::RtpDumpWriter> dump_writer_; I'm wondering about how large the data can be? We should limit it (also if we write it to disk), and doing that, should we use a circular buffer? If it's not expected to be too large, we could just keep it in memory and write it to disk in the uploader.
https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS#ne... chrome/browser/media/DEPS:5: "+third_party/libjingle", On 2014/05/05 14:38:03, Henrik Grunell wrote: > I'm not so sure we should depend on libjingle here. It's for the RTP dump > writer, right? Correct. It already depends on third_party/webrtc, so I think it should be fine for third_party/libjingle. Otherwise, we need to duplicate the code of RtpDumpWriter in Chrome. https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:62: virtual void OnIncomingRtpPacket(const uint8* packet, size_t length) OVERRIDE; On 2014/05/05 14:38:03, Henrik Grunell wrote: > Who will call these? From the socket classes in content/browser/renderer_host/p2p https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:81: scoped_ptr<talk_base::FifoBuffer> dump_buffer_; On 2014/05/05 14:38:03, Henrik Grunell wrote: > Does it have to be a talk_base::FifoBuffer? MemoryBufferBase is enough for the use, but it doesn't have a public ctor, so I'll need to change Libjingle. I can certainly make the change. https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:82: scoped_ptr<cricket::RtpDumpWriter> dump_writer_; On 2014/05/05 14:38:03, Henrik Grunell wrote: > I'm wondering about how large the data can be? We should limit it (also if we > write it to disk), and doing that, should we use a circular buffer? If it's not > expected to be too large, we could just keep it in memory and write it to disk > in the uploader. For 2Mbps video, there are 250 packets/s; for 40kbps audio, there are 50 packets/s. So if each RTP header is 12 bytes uncompressed, it will be 12 * 300 = 3600 bytes/s for just one audio+video stream. If there are multiple steams, say 10, it can go over 1 MB within a minute. So I think it's too large to hold all in memory, even compressed assuming a 10:1 compression rate. Limiting the on-disk file size makes sense of course.
Adding tommi for more feedback on the overall design.
The general design and data pipeline looks fine to me. https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS#ne... chrome/browser/media/DEPS:5: "+third_party/libjingle", On 2014/05/05 16:50:56, jiayl wrote: > On 2014/05/05 14:38:03, Henrik Grunell wrote: > > I'm not so sure we should depend on libjingle here. It's for the RTP dump > > writer, right? > > Correct. It already depends on third_party/webrtc, so I think it should be fine > for third_party/libjingle. Otherwise, we need to duplicate the code of > RtpDumpWriter in Chrome. Yes, that's of course more convenient. It's probably fine, but I suggest you check with someone (Tommi?) who knows more about the include rules. Maybe it can be limited to the specific folder the rtp writer resides in? https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/webrtc_... chrome/browser/media/webrtc_rtp_dump_handler.h:62: virtual void OnIncomingRtpPacket(const uint8* packet, size_t length) OVERRIDE; On 2014/05/05 16:50:56, jiayl wrote: > On 2014/05/05 14:38:03, Henrik Grunell wrote: > > Who will call these? > > From the socket classes in content/browser/renderer_host/p2p Through RenderProcessHostImpl as for the log messages and as I saw now that you wrote this in the description?
https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS File chrome/browser/media/DEPS (right): https://codereview.chromium.org/264793017/diff/1/chrome/browser/media/DEPS#ne... chrome/browser/media/DEPS:5: "+third_party/libjingle", On 2014/05/06 08:34:26, Henrik Grunell wrote: > On 2014/05/05 16:50:56, jiayl wrote: > > On 2014/05/05 14:38:03, Henrik Grunell wrote: > > > I'm not so sure we should depend on libjingle here. It's for the RTP dump > > > writer, right? > > > > Correct. It already depends on third_party/webrtc, so I think it should be > fine > > for third_party/libjingle. Otherwise, we need to duplicate the code of > > RtpDumpWriter in Chrome. > > Yes, that's of course more convenient. It's probably fine, but I suggest you > check with someone (Tommi?) who knows more about the include rules. Maybe it can > be limited to the specific folder the rtp writer resides in? We're depending on libjingle on chromeos so I guess this is alright. Is there no way to do this in the renderer process instead? Adding a third party library to the browser makes it harder to protect against problems.
Removed the DEPS change; Added WebRtcRtpDumpWriter, which is mostly ported from libjingle; Made it possible to limit the dump file size. PTAL. If there is no major design problem, I'll complete the impl and add tests.
I think the general design looks good. I stopped reviewing the details when I saw in the last comment you'd finish the implementation. Continued with higher level comments. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_logging_handler_host.cc:228: void WebRtcLoggingHandlerHost::OnRtpPacket(const uint8* packet, Check thread. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_logging_handler_host.h:77: void OnRtpPacket(const uint8* packet, size_t length, bool incoming); Add comment. Which thread should it be called on? https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_logging_handler_host.h:170: scoped_ptr<WebRtcRtpDumpHandler> rtp_dump_handler_; Add comment. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.cc:21: WebRtcRtpDumpHandler::~WebRtcRtpDumpHandler() { On which thread should this class be created and deleted on? Not IO? If yes, you could use a ThreadChecker instead of BrowserThread::CurrentlyOn(BrowserThread::IO and don't need the DeleteSoon. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.cc:37: if (type.incoming && incoming_state_ == STATE_NONE) { If both incoming and outgoing are set, incoming state is NONE and outgoing state is not NONE, |succeeded| will be true which it shouldn't, right? It should fail. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.h:36: // |dump_dir| as "rtpdump_XXXXX.gz". Explain in the comment what XXXXX is. How is filename uniqueness ensured across tabs and streams? https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.h:58: virtual void OnRtpPacket(const uint8* packet, size_t length, bool incoming); Add comment. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.h:72: void OnMaxDumpSizeReached(); My guess before looking at the implementation is that |dump_writer_| calls these. Maybe comment these? https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_handler.h:76: base::FilePath dump_dir_; Comment all member variables. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_writer.cc:168: // OnFlushDone is necessary to avoid running the callback after this object is Will this object delete the file if this happens? https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_writer.cc:188: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); OK, I see you're doing it on the FILE thread. https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/40001/chrome/browser/media/web... chrome/browser/media/webrtc_rtp_dump_writer.h:27: // This object must run on the IO thread. You can't write to file on the IO thread, you'll need to do that on the FILE thread. https://codereview.chromium.org/264793017/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/264793017/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1104: 'browser/media/webrtc_rtp_dump_handler.cc', Exclude files if webrtc isn't enabled. Same in other gypi files.
PTAL! Henrik: webrtc_log*, webrtc_rtp_dump* tommi: owner of chrome/browser/media mallinath: content/browser/renderer_host/p2p/* jam: content/public/*
First round of review. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:182: post_data.get(), compressed_log, upload_done_data.rtp_dumps, meta_data); |upload_done_data| contains info that's needed after the upload is finished, i.e. in WebRtcLogUploader::OnURLFetchComplete. Pass the rtp dump data in it's own argument in LoggingStoppedDoUpload(). https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:269: break; Maybe log a warning? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:58: const size_t kWebRtcLogSize = 4 * 1024 * 1024; // 4 MB What's the rationale for decreasing this? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:239: FireGenericDoneCallback(&start_callback, false, ""); Add an error message (third argument) for all places and cases were an error is returned. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:244: content::BrowserThread::PostTaskAndReplyWithResult( What happens if there's another call to StartRtpDump while on the file thread? Seems like a potential problem. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:257: WebRtcRtpDumpHandler::PacketType type = {incoming, outgoing}; I think this part should be moved to it's own function and CreateRtpDumpHandlerAndStart calls that function instead of calling StartRtpDump again. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:280: void WebRtcLoggingHandlerHost::OnRtpPacket(const uint8* packet_header, Which thread should this be called on? Thread check? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:305: if (rtp_dump_handler_) { Is |rtp_dump_handler_| expected to be NULL sometimes? When? If not, DCHECK that is exists instead. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:526: if (rtp_dump_handler_ && Same here, can it be NULL? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:527: rtp_dump_handler_->ReleaseDump(base::Bind( So the dump handler will call DoUpload in ReleaseDump. Seems unnecessarily complicated. Can it be simplified (return dumps?) or why is this needed? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:93: // Called when a RTP packet is sent or received. Can be called on any thread. Nit: an RTP packet. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:150: void TriggerUploadLogAndRtpDump(const base::FilePath& log_directory); Maybe just TriggerUpload? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:156: void CreateRtpDumpHandlerAndStart(bool incoming, I think comments on all added functions are good. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:161: void DoUpload(const base::FilePath& log_directory, What will this upload? Can the name be changed to clarify what it does? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:203: scoped_ptr<WebRtcRtpDumpHandler> rtp_dump_handler_; Add comment.
looked at chrome/browser/media/* lgtm with one request. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; can these members be const?
Addressed Henrik's comments. PTAL. The new patch is created after a sync, so it has some unrelated changes compared to the last version. Sorry about that, but I have to sync to make it compile after a branch switch. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:182: post_data.get(), compressed_log, upload_done_data.rtp_dumps, meta_data); On 2014/05/13 09:12:31, Henrik Grunell wrote: > |upload_done_data| contains info that's needed after the upload is finished, > i.e. in WebRtcLogUploader::OnURLFetchComplete. Pass the rtp dump data in it's > own argument in LoggingStoppedDoUpload(). The dump paths are needed to add them to the chrome:webrtc-logs page, just like the log path. I moved the TODO to OnURLFetchComplete to make it clear. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:269: break; On 2014/05/13 09:12:31, Henrik Grunell wrote: > Maybe log a warning? Done. Also changed RtpDumpHandler and RtpDumpWriter to not include empty/non-exiting dump file. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; On 2014/05/13 15:19:55, tommi wrote: > can these members be const? The assignment operator (required to use with std::vector) will not work if they are const. But I went through other new classes and made the members const when possible. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:58: const size_t kWebRtcLogSize = 4 * 1024 * 1024; // 4 MB On 2014/05/13 09:12:31, Henrik Grunell wrote: > What's the rationale for decreasing this? The max upload size including log and dumps is 10,000KB. So I changed it to make the total max size below 10MB. But I forgot that this is the uncompressed size. So 6MB should be fine. I changed it back. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:239: FireGenericDoneCallback(&start_callback, false, ""); On 2014/05/13 09:12:31, Henrik Grunell wrote: > Add an error message (third argument) for all places and cases were an error is > returned. Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:244: content::BrowserThread::PostTaskAndReplyWithResult( On 2014/05/13 09:12:31, Henrik Grunell wrote: > What happens if there's another call to StartRtpDump while on the file thread? > Seems like a potential problem. Changed CreateRtpDumpHandlerAndStart to check if rtp_dump_handler_ is NULL. If it's not NULL, do not re-create it. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:257: WebRtcRtpDumpHandler::PacketType type = {incoming, outgoing}; On 2014/05/13 09:12:31, Henrik Grunell wrote: > I think this part should be moved to it's own function and > CreateRtpDumpHandlerAndStart calls that function instead of calling StartRtpDump > again. Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:280: void WebRtcLoggingHandlerHost::OnRtpPacket(const uint8* packet_header, On 2014/05/13 09:12:31, Henrik Grunell wrote: > Which thread should this be called on? Thread check? In practice it called on the browser UI thread. Since it always posts to the IO thread, the interface does not enforce the thread it's called on. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:305: if (rtp_dump_handler_) { On 2014/05/13 09:12:31, Henrik Grunell wrote: > Is |rtp_dump_handler_| expected to be NULL sometimes? When? If not, DCHECK that > is exists instead. The call is triggered by P2PSocketHost, which lives in a different module (i.e. content) and running on different threads. So I want to be defensive even in the current implementation it should be not NULL. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:526: if (rtp_dump_handler_ && On 2014/05/13 09:12:31, Henrik Grunell wrote: > Same here, can it be NULL? It can be NULL if rtp dump is never started. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:527: rtp_dump_handler_->ReleaseDump(base::Bind( On 2014/05/13 09:12:31, Henrik Grunell wrote: > So the dump handler will call DoUpload in ReleaseDump. Seems unnecessarily > complicated. Can it be simplified (return dumps?) or why is this needed? Now Refactored to end the dump on StopDump. So ReleaseDump returns the file paths directly. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:93: // Called when a RTP packet is sent or received. Can be called on any thread. On 2014/05/13 09:12:31, Henrik Grunell wrote: > Nit: an RTP packet. Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:150: void TriggerUploadLogAndRtpDump(const base::FilePath& log_directory); On 2014/05/13 09:12:31, Henrik Grunell wrote: > Maybe just TriggerUpload? Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:156: void CreateRtpDumpHandlerAndStart(bool incoming, On 2014/05/13 09:12:31, Henrik Grunell wrote: > I think comments on all added functions are good. Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:161: void DoUpload(const base::FilePath& log_directory, On 2014/05/13 09:12:31, Henrik Grunell wrote: > What will this upload? Can the name be changed to clarify what it does? Done. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:203: scoped_ptr<WebRtcRtpDumpHandler> rtp_dump_handler_; On 2014/05/13 09:12:31, Henrik Grunell wrote: > Add comment. Done.
On 2014/05/13 21:48:15, jiayl wrote: > Addressed Henrik's comments. PTAL. > > The new patch is created after a sync, so it has some unrelated changes compared > to the last version. Sorry about that, but I have to sync to make it compile > after a branch switch. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > File chrome/browser/media/webrtc_log_uploader.cc (right): > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_log_uploader.cc:182: post_data.get(), > compressed_log, upload_done_data.rtp_dumps, meta_data); > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > |upload_done_data| contains info that's needed after the upload is finished, > > i.e. in WebRtcLogUploader::OnURLFetchComplete. Pass the rtp dump data in it's > > own argument in LoggingStoppedDoUpload(). > > The dump paths are needed to add them to the chrome:webrtc-logs page, just like > the log path. I moved the TODO to OnURLFetchComplete to make it clear. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_log_uploader.cc:269: break; > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Maybe log a warning? > > Done. Also changed RtpDumpHandler and RtpDumpWriter to not include > empty/non-exiting dump file. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > File chrome/browser/media/webrtc_log_uploader.h (right): > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_log_uploader.h:33: std::string name; > On 2014/05/13 15:19:55, tommi wrote: > > can these members be const? > > The assignment operator (required to use with std::vector) will not work if they > are const. But I went through other new classes and made the members const when > possible. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > File chrome/browser/media/webrtc_logging_handler_host.cc (right): > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:58: const size_t > kWebRtcLogSize = 4 * 1024 * 1024; // 4 MB > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > What's the rationale for decreasing this? > > The max upload size including log and dumps is 10,000KB. So I changed it to make > the total max size below 10MB. But I forgot that this is the uncompressed size. > So 6MB should be fine. I changed it back. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:239: > FireGenericDoneCallback(&start_callback, false, ""); > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Add an error message (third argument) for all places and cases were an error > is > > returned. > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:244: > content::BrowserThread::PostTaskAndReplyWithResult( > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > What happens if there's another call to StartRtpDump while on the file thread? > > Seems like a potential problem. > > Changed CreateRtpDumpHandlerAndStart to check if rtp_dump_handler_ is NULL. If > it's not NULL, do not re-create it. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:257: > WebRtcRtpDumpHandler::PacketType type = {incoming, outgoing}; > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > I think this part should be moved to it's own function and > > CreateRtpDumpHandlerAndStart calls that function instead of calling > StartRtpDump > > again. > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:280: void > WebRtcLoggingHandlerHost::OnRtpPacket(const uint8* packet_header, > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Which thread should this be called on? Thread check? > > In practice it called on the browser UI thread. Since it always posts to the IO > thread, the interface does not enforce the thread it's called on. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:305: if (rtp_dump_handler_) > { > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Is |rtp_dump_handler_| expected to be NULL sometimes? When? If not, DCHECK > that > > is exists instead. > > The call is triggered by P2PSocketHost, which lives in a different module (i.e. > content) and running on different threads. So I want to be defensive even in the > current implementation it should be not NULL. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:526: if (rtp_dump_handler_ > && > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Same here, can it be NULL? > > It can be NULL if rtp dump is never started. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.cc:527: > rtp_dump_handler_->ReleaseDump(base::Bind( > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > So the dump handler will call DoUpload in ReleaseDump. Seems unnecessarily > > complicated. Can it be simplified (return dumps?) or why is this needed? > > Now Refactored to end the dump on StopDump. So ReleaseDump returns the file > paths directly. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > File chrome/browser/media/webrtc_logging_handler_host.h (right): > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.h:93: // Called when a RTP > packet is sent or received. Can be called on any thread. > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Nit: an RTP packet. > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.h:150: void > TriggerUploadLogAndRtpDump(const base::FilePath& log_directory); > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Maybe just TriggerUpload? > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.h:156: void > CreateRtpDumpHandlerAndStart(bool incoming, > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > I think comments on all added functions are good. > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.h:161: void DoUpload(const > base::FilePath& log_directory, > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > What will this upload? Can the name be changed to clarify what it does? > > Done. > > https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... > chrome/browser/media/webrtc_logging_handler_host.h:203: > scoped_ptr<WebRtcRtpDumpHandler> rtp_dump_handler_; > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Add comment. > > Done. minor nits, otherwise LGTM for p2p sockets.
https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... content/browser/renderer_host/p2p/socket_dispatcher_host.cc:155: if (dump_incoming_rtp_packet_ != incoming || Do we really need if condition here? Let socket decide how to handle these values. wdyt? https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... content/browser/renderer_host/p2p/socket_dispatcher_host.cc:166: if (dump_incoming_rtp_packet_ == incoming || same as above. https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:550: return; add a DCHECK before returning.
Nice work on this Jiayang. Another round. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:182: post_data.get(), compressed_log, upload_done_data.rtp_dumps, meta_data); On 2014/05/13 21:48:17, jiayl wrote: > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > |upload_done_data| contains info that's needed after the upload is finished, > > i.e. in WebRtcLogUploader::OnURLFetchComplete. Pass the rtp dump data in it's > > own argument in LoggingStoppedDoUpload(). > > The dump paths are needed to add them to the chrome:webrtc-logs page, just like > the log path. I moved the TODO to OnURLFetchComplete to make it clear. Ah, right, it's the path and not the actual data in that struct. OK, it's fine. (However, I think UploadDoneData is subject of change or renaming, but that another story.) https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:58: const size_t kWebRtcLogSize = 4 * 1024 * 1024; // 4 MB On 2014/05/13 21:48:17, jiayl wrote: > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > What's the rationale for decreasing this? > > The max upload size including log and dumps is 10,000KB. So I changed it to make > the total max size below 10MB. But I forgot that this is the uncompressed size. > So 6MB should be fine. I changed it back. OK. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:244: content::BrowserThread::PostTaskAndReplyWithResult( On 2014/05/13 21:48:17, jiayl wrote: > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > What happens if there's another call to StartRtpDump while on the file thread? > > Seems like a potential problem. > > Changed CreateRtpDumpHandlerAndStart to check if rtp_dump_handler_ is NULL. If > it's not NULL, do not re-create it. So, it's safe to call multiple times? https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:305: if (rtp_dump_handler_) { On 2014/05/13 21:48:17, jiayl wrote: > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > Is |rtp_dump_handler_| expected to be NULL sometimes? When? If not, DCHECK > that > > is exists instead. > > The call is triggered by P2PSocketHost, which lives in a different module (i.e. > content) and running on different threads. So I want to be defensive even in the > current implementation it should be not NULL. This sounds dangerous. Do you mean it can be a race when stopping? The callback to this object shouldn't be set if not enabled, right? If you suspect a race, make sure there isn't one. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:301: host->StartRtpDump(params->incoming, params->outgoing); Can this fail? Does it need to finish before we send the completion callback to the extension? (I saw in RenderProcessHostImpl that it posts a task.) For example, if whatever is done in that task hasn't finished when StopRtpDump is called here, is that OK? Maybe RPH should responsible for calling LHH and we will only need RPH from here? Not sure if that's better. Either way, I think it should be documented how this works. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:238: FireGenericDoneCallback(&start_callback, false, "Invalid argument."); This will end up in the javascript app, I think that as much details as possible is good for troubleshooting. What about "Either incoming or outgoing argument must be set." Thinking of this more, it would be best to have an enum {INCOMING, OUTGOING, INCOMING_AND_OUTGOING}. Then this check isn't needed. Does the extension API have two arguments? It should also be changed imo. Wdyt? https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:280: // Can be called on any thread as it always posts to the IO thread. Move this comment to the declaration. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:528: if (rtp_dump_handler_) { Remove {} https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:66: g_ongoing_rtp_dumps--; nit: --g_ongoing_rtp_dumps; https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:27: // Must be created/used/destroyed on the IO thread. I think you should have a ThreadChecker or make it a NonThreadSafe (which uses a ThreadChecker). https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:32: struct PacketType { Maybe an enum with three values instead since {false, false} isn't valid? https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:113: void SetDumpWriterForTesting(scoped_ptr<WebRtcRtpDumpWriter> writer); Can the writer be injected in the constructor instead? https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:115: const base::FilePath dump_dir_; Comment all member variables. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:120: scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; So there's one writer that for both incoming and outgoing. It seems that incoming and outgoing can be handled completely separately, maybe there can be two dump handlers, one for incoming and one for outgoing? We'll get rid of all incoming_* and outgoing_*. Could they have a dump writer each? Or is it tightly coupled together? Maybe share an injected writer? Lifetime must be ensured in that case. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:39: WebRtcRtpDumpWriter(const base::FilePath& incoming_dump_path, I think this class should handle one dump only and two objects be created if both incoming and outgoing is desired.
https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_dispatcher_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... content/browser/renderer_host/p2p/socket_dispatcher_host.cc:155: if (dump_incoming_rtp_packet_ != incoming || On 2014/05/14 00:57:01, mallinath2 wrote: > Do we really need if condition here? Let socket decide how to handle these > values. wdyt? The check is to avoid unnecessary work if no change is made to the dump status. We don't need to iterate the container if nothing needs to be changed.
Some clarifications. I'll update the patch for the other comments. Thanks! https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:244: content::BrowserThread::PostTaskAndReplyWithResult( On 2014/05/14 12:14:12, Henrik Grunell wrote: > On 2014/05/13 21:48:17, jiayl wrote: > > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > > What happens if there's another call to StartRtpDump while on the file > thread? > > > Seems like a potential problem. > > > > Changed CreateRtpDumpHandlerAndStart to check if rtp_dump_handler_ is NULL. If > > it's not NULL, do not re-create it. > > So, it's safe to call multiple times? yes, it's safe. https://codereview.chromium.org/264793017/diff/110001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:305: if (rtp_dump_handler_) { On 2014/05/14 12:14:12, Henrik Grunell wrote: > On 2014/05/13 21:48:17, jiayl wrote: > > On 2014/05/13 09:12:31, Henrik Grunell wrote: > > > Is |rtp_dump_handler_| expected to be NULL sometimes? When? If not, DCHECK > > that > > > is exists instead. > > > > The call is triggered by P2PSocketHost, which lives in a different module > (i.e. > > content) and running on different threads. So I want to be defensive even in > the > > current implementation it should be not NULL. > > This sounds dangerous. Do you mean it can be a race when stopping? The callback > to this object shouldn't be set if not enabled, right? If you suspect a race, > make sure there isn't one. There is no race in the impl. What I mean is that the client is not required to maintain the dumping state, instead WebRtcLoggingHandlerHost/WebRtcRtpDumpHandler should ignore the packet if dumping is not started or stopped. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:301: host->StartRtpDump(params->incoming, params->outgoing); On 2014/05/14 12:14:12, Henrik Grunell wrote: > Can this fail? Does it need to finish before we send the completion callback to > the extension? (I saw in RenderProcessHostImpl that it posts a task.) For > example, if whatever is done in that task hasn't finished when StopRtpDump is > called here, is that OK? > > Maybe RPH should responsible for calling LHH and we will only need RPH from > here? Not sure if that's better. Either way, I think it should be documented how > this works. It cannot fail and we do not need to block the callback. LHH (in chrome) is not a known type to RPH (in content). We'll need a new content public interface to make RPH directly call LHH. I'd prefer the current way. I'll add comments to explain. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:120: scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; On 2014/05/14 12:14:12, Henrik Grunell wrote: > So there's one writer that for both incoming and outgoing. It seems that > incoming and outgoing can be handled completely separately, maybe there can be > two dump handlers, one for incoming and one for outgoing? We'll get rid of all > incoming_* and outgoing_*. Could they have a dump writer each? Or is it tightly > coupled together? Maybe share an injected writer? Lifetime must be ensured in > that case. It'll be more difficult to apply the max dump limit if the writers are separated, because the limit is on the total size of all dumps.
PTAL! Thanks! https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/110001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:550: return; On 2014/05/14 00:57:01, mallinath2 wrote: > add a DCHECK before returning. Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:238: FireGenericDoneCallback(&start_callback, false, "Invalid argument."); On 2014/05/14 12:14:12, Henrik Grunell wrote: > This will end up in the javascript app, I think that as much details as possible > is good for troubleshooting. What about "Either incoming or outgoing argument > must be set." > > Thinking of this more, it would be best to have an enum {INCOMING, OUTGOING, > INCOMING_AND_OUTGOING}. Then this check isn't needed. Does the extension API > have two arguments? It should also be changed imo. Wdyt? Done improving the error message. How could we change the extension API to pass an enum? Does that require creating a new JS enum type? https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:280: // Can be called on any thread as it always posts to the IO thread. On 2014/05/14 12:14:12, Henrik Grunell wrote: > Move this comment to the declaration. Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:528: if (rtp_dump_handler_) { On 2014/05/14 12:14:12, Henrik Grunell wrote: > Remove {} Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:66: g_ongoing_rtp_dumps--; On 2014/05/14 12:14:12, Henrik Grunell wrote: > nit: --g_ongoing_rtp_dumps; Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:27: // Must be created/used/destroyed on the IO thread. On 2014/05/14 12:14:12, Henrik Grunell wrote: > I think you should have a ThreadChecker or make it a NonThreadSafe (which uses a > ThreadChecker). It's a stronger requirement than NonThreadSafe or ThreadChecker. It has to be run on the unique browser thread to avoid racing on the global dump count (g_ongoing_rtp_dumps). https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:32: struct PacketType { On 2014/05/14 12:14:12, Henrik Grunell wrote: > Maybe an enum with three values instead since {false, false} isn't valid? Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:113: void SetDumpWriterForTesting(scoped_ptr<WebRtcRtpDumpWriter> writer); On 2014/05/14 12:14:12, Henrik Grunell wrote: > Can the writer be injected in the constructor instead? It cannot. Because the writer has to be created with the max size callback bound to the handler. So the handler must be created before the writer is created. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:115: const base::FilePath dump_dir_; On 2014/05/14 12:14:12, Henrik Grunell wrote: > Comment all member variables. Done. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:39: WebRtcRtpDumpWriter(const base::FilePath& incoming_dump_path, On 2014/05/14 12:14:12, Henrik Grunell wrote: > I think this class should handle one dump only and two objects be created if > both incoming and outgoing is desired. See my other reply about max size limit.
https://codereview.chromium.org/264793017/diff/190001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/190001/content/public/browser/... content/public/browser/render_process_host.h:216: virtual void StartRtpDump(bool incoming, bool outgoing) = 0; can you simplify this api by: 1) removing teh SetWebRtcRtpPacketCallback method and instead passing that callback to StartRtpDump 2) removing StopRtpDump by having StartRtpDump return a callback that is used to stop? 3) remove OnRtpPacke from RenderProcessHostImpl and instead have the webrtc code use the callback directly instead of going through RPHImpl
https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:301: host->StartRtpDump(params->incoming, params->outgoing); On 2014/05/14 16:07:58, jiayl wrote: > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > Can this fail? Does it need to finish before we send the completion callback > to > > the extension? (I saw in RenderProcessHostImpl that it posts a task.) For > > example, if whatever is done in that task hasn't finished when StopRtpDump is > > called here, is that OK? > > > > Maybe RPH should responsible for calling LHH and we will only need RPH from > > here? Not sure if that's better. Either way, I think it should be documented > how > > this works. > > It cannot fail and we do not need to block the callback. > > LHH (in chrome) is not a known type to RPH (in content). We'll need a new > content public interface to make RPH directly call LHH. I'd prefer the current > way. > > I'll add comments to explain. OK. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:238: FireGenericDoneCallback(&start_callback, false, "Invalid argument."); On 2014/05/14 18:59:12, jiayl wrote: > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > This will end up in the javascript app, I think that as much details as > possible > > is good for troubleshooting. What about "Either incoming or outgoing argument > > must be set." > > > > Thinking of this more, it would be best to have an enum {INCOMING, OUTGOING, > > INCOMING_AND_OUTGOING}. Then this check isn't needed. Does the extension API > > have two arguments? It should also be changed imo. Wdyt? > > > Done improving the error message. > > How could we change the extension API to pass an enum? Does that require > creating a new JS enum type? I'd like a JS enum used in the JS API. We could do a follow-up for that and only use an enum internally. Then convert in the extension API implementation and check for both not false. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:120: scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; On 2014/05/14 16:07:58, jiayl wrote: > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > So there's one writer that for both incoming and outgoing. It seems that > > incoming and outgoing can be handled completely separately, maybe there can be > > two dump handlers, one for incoming and one for outgoing? We'll get rid of all > > incoming_* and outgoing_*. Could they have a dump writer each? Or is it > tightly > > coupled together? Maybe share an injected writer? Lifetime must be ensured in > > that case. > > It'll be more difficult to apply the max dump limit if the writers are > separated, because > the limit is on the total size of all dumps. I see. And the incoming and outgoing can't be expected to be ~equal. OK, let's keep it this way. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:159: void CreateRtpDumpHandlerAndStart(bool incoming, Can you use an enum instead of two bools? https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:18: // |path| must be a copy of FilePath, not a reference. Explain why not a reference in the comment. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:57: : dump_dir_(dump_dir), Is |dump_dir_| ever checked to be existing? This should be done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:67: BrowserThread::PostTask(BrowserThread::FILE, How is it guaranteed that the writer stops writing before the file is deleted? Looks like that could happen. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:90: succeeded = true; I think this should only succeed if both types could be started. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:97: if (!dump_writer_ && g_ongoing_rtp_dumps >= kMaxOngoingRtpDumpsAllowed) { Do this check before the state checks above. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:168: dump_writer_->EndDump( Can the EndDump be changed so that it's called once only? The logic gets a bit complicated when only one callback to the LHH must be ensured. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:171: base::Unretained(this), Why is unretained safe? (Comment on this at all places used or a general comment on top.) https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:41: static bool TypeContainsIncoming(PacketType type); Can these be made ordinary functions in the implementation file? https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:55: // - if type.incoming == true, incoming dumping has not been started, and Update comment for enum change.
PTAL! Thanks. https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:238: FireGenericDoneCallback(&start_callback, false, "Invalid argument."); On 2014/05/15 14:36:34, Henrik Grunell wrote: > On 2014/05/14 18:59:12, jiayl wrote: > > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > > This will end up in the javascript app, I think that as much details as > > possible > > > is good for troubleshooting. What about "Either incoming or outgoing > argument > > > must be set." > > > > > > Thinking of this more, it would be best to have an enum {INCOMING, OUTGOING, > > > INCOMING_AND_OUTGOING}. Then this check isn't needed. Does the extension API > > > have two arguments? It should also be changed imo. Wdyt? > > > > > > Done improving the error message. > > > > How could we change the extension API to pass an enum? Does that require > > creating a new JS enum type? > > I'd like a JS enum used in the JS API. We could do a follow-up for that and only > use an enum internally. Then convert in the extension API implementation and > check for both not false. Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.h:159: void CreateRtpDumpHandlerAndStart(bool incoming, On 2014/05/15 14:36:34, Henrik Grunell wrote: > Can you use an enum instead of two bools? Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:18: // |path| must be a copy of FilePath, not a reference. On 2014/05/15 14:36:34, Henrik Grunell wrote: > Explain why not a reference in the comment. I was wrong. It doesn't need to be a copy. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:57: : dump_dir_(dump_dir), On 2014/05/15 14:36:34, Henrik Grunell wrote: > Is |dump_dir_| ever checked to be existing? This should be done. it's stated in the header file that the caller must ensure the dir exists. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:67: BrowserThread::PostTask(BrowserThread::FILE, On 2014/05/15 14:36:34, Henrik Grunell wrote: > How is it guaranteed that the writer stops writing before the file is deleted? > Looks like that could happen. Fixed. Reset the writer first will guarantee that. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:90: succeeded = true; On 2014/05/15 14:36:34, Henrik Grunell wrote: > I think this should only succeed if both types could be started. Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:97: if (!dump_writer_ && g_ongoing_rtp_dumps >= kMaxOngoingRtpDumpsAllowed) { On 2014/05/15 14:36:34, Henrik Grunell wrote: > Do this check before the state checks above. Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:168: dump_writer_->EndDump( On 2014/05/15 14:36:34, Henrik Grunell wrote: > Can the EndDump be changed so that it's called once only? The logic gets a bit > complicated when only one callback to the LHH must be ensured. Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:171: base::Unretained(this), On 2014/05/15 14:36:34, Henrik Grunell wrote: > Why is unretained safe? (Comment on this at all places used or a general comment > on top.) Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:41: static bool TypeContainsIncoming(PacketType type); On 2014/05/15 14:36:34, Henrik Grunell wrote: > Can these be made ordinary functions in the implementation file? Done. https://codereview.chromium.org/264793017/diff/190001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:55: // - if type.incoming == true, incoming dumping has not been started, and On 2014/05/15 14:36:34, Henrik Grunell wrote: > Update comment for enum change. Done. https://codereview.chromium.org/264793017/diff/190001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/190001/content/public/browser/... content/public/browser/render_process_host.h:216: virtual void StartRtpDump(bool incoming, bool outgoing) = 0; On 2014/05/14 23:59:35, jam wrote: > can you simplify this api by: > 1) removing teh SetWebRtcRtpPacketCallback method and instead passing that > callback to StartRtpDump > > 2) removing StopRtpDump by having StartRtpDump return a callback that is used to > stop? > > 3) remove OnRtpPacke from RenderProcessHostImpl and instead have the webrtc code > use the callback directly instead of going through RPHImpl Great idea! Done.
https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:510: packet_dump_callback_.Reset(); actually, how is this method thread safe to call per the RPH document? it doesn't appear to be https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... content/public/browser/render_process_host.h:225: // used to stop dumping. This method can be called on any thread. The returned can you clarify what you mean by "called on any thread"? you mean the callback, not StartRtpDump right? https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... content/public/browser/render_process_host.h:228: RtpDumpType, can you keep it as with booleans as before for incoming & outgoing? in the content api we put each enum in a separate file, so making it here for these two would bloat content/public/browser even more
PTAL https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:510: packet_dump_callback_.Reset(); On 2014/05/16 22:03:22, jam wrote: > actually, how is this method thread safe to call per the RPH document? it > doesn't appear to be It's essentially WebRtcLoggingHandlerHost::OnRtpPacket, which always posts to IO thread and WebRtcLoggingHandlerHost is thread safe ref counted. So I think it's OK call it from any thread. https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... content/public/browser/render_process_host.h:225: // used to stop dumping. This method can be called on any thread. The returned On 2014/05/16 22:03:22, jam wrote: > can you clarify what you mean by "called on any thread"? you mean the callback, > not StartRtpDump right? Improved the comment. https://codereview.chromium.org/264793017/diff/260001/content/public/browser/... content/public/browser/render_process_host.h:228: RtpDumpType, On 2014/05/16 22:03:22, jam wrote: > can you keep it as with booleans as before for incoming & outgoing? in the > content api we put each enum in a separate file, so making it here for these two > would bloat content/public/browser even more Done.
not lgtm - sorry, I realized I missed some of the files last time. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/rt... File chrome/browser/media/rtp_dump_type.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/rt... chrome/browser/media/rtp_dump_type.h:8: enum RtpDumpType { documentation https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:45: post_data->append("\r\n"); join this and the next line into a single call. less code bloat. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:48: post_data->append("\""); this and the next line joined https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:51: post_data->append("\"\r\n"); same here. If you would prefer this on separate lines for readability, you can do something like: post_data->append("\"\r\n" "Content-Type: application/gzip\r\n\r\n"); same throughout https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:274: if (base::ReadFileToString(it->path, &dump_data)) { nit: no {} https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; would be good to document these. what does the path represent? and name of what? I'd still like to make these members const and I think we can do that by not using std::vector. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:44: std::vector<WebRtcRtpDumpDescription> rtp_dumps; Instead of implicitly adding data copy, can we use ScopedVector instead? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:110: const std::vector<WebRtcRtpDumpDescription>& rtp_dumps, ScopedVector? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:552: if (!rtp_dump_handler_) if this is already non-NULL, is that something unexpected? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:572: WebRtcLogUploadDoneData upload_done_data; can WebRtcLogUploadDoneData be a ScopedVector? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:18: void DeleteFileHelper(const base::FilePath& path) { nit: is this helper needed? (can we bind to DeleteFile directly? there's a helper in base to ignore the return value if that's the problem) https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:34: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); nice job on the thread checks! https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; {} https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:284: g_ongoing_rtp_dumps--; nit: --g_ongoing_rtp_dumps; https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:32: base::FilePath incoming_dump_path; can these be const? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:43: virtual ~WebRtcRtpDumpHandler(); why virtual? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc:88: EXPECT_GT(base::WriteFile(*incoming_dump, dummy, arraysize(dummy)), 0); is writing the terminating \0 intentional? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:18: struct RtpDumpFileHeader { is there a certain packing that's assumed for these structs? If so, what is it? Is it safe to assume that all build configurations will be configured for that exact packing or should we be explicit? Maybe it would be good to have compile time assert for the sizes of these structs? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:50: uint32 start_sec; // start of recording, the seconds part. could these members all be const? (sorry, have to ask) https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:85: uint16 packet_dump_length; and these ones? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:101: for (size_t i = 0; i < src_len; ++i) { nit: no {} https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:190: bool Compress(std::vector<uint8>* input, std::vector<uint8>* output) { why is |input| writable? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:287: incoming_file_thread_worker_.reset(); if we get here, is that an error or expected case? Can you document what will have happened if we hit here? If this should never happen, I'd prefer a check here and just pass the return value of relese() to DeleteSoon(). Same below. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:62: size_t max_dump_size() const { return max_dump_size_; } missing dcheck for IO thread? Maybe this should just be implemented in the cc file then. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:83: return true; nit: this always returns true... just make it return void since it encapsulates all the expectations? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:182: dump_pos += 2; += sizeof(uint16); ? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:190: dump_pos += 2; same here https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:193: dump_pos += 4; size of the time field? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:229: { what is the scope for? https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:254: { same here and throughout (I'm sure I'm missing something, but if I'm not seeing it, it might be worth documenting)
https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/rt... File chrome/browser/media/rtp_dump_type.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/rt... chrome/browser/media/rtp_dump_type.h:8: enum RtpDumpType { On 2014/05/17 10:18:37, tommi wrote: > documentation Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:45: post_data->append("\r\n"); On 2014/05/17 10:18:37, tommi wrote: > join this and the next line into a single call. less code bloat. Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:48: post_data->append("\""); On 2014/05/17 10:18:37, tommi wrote: > this and the next line joined Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:51: post_data->append("\"\r\n"); On 2014/05/17 10:18:37, tommi wrote: > same here. > > If you would prefer this on separate lines for readability, you can do something > like: > > post_data->append("\"\r\n" > "Content-Type: application/gzip\r\n\r\n"); > > same throughout Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:274: if (base::ReadFileToString(it->path, &dump_data)) { On 2014/05/17 10:18:37, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; On 2014/05/17 10:18:37, tommi wrote: > would be good to document these. what does the path represent? and name of > what? > > I'd still like to make these members const and I think we can do that by not > using std::vector. Added comments. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:44: std::vector<WebRtcRtpDumpDescription> rtp_dumps; On 2014/05/17 10:18:37, tommi wrote: > Instead of implicitly adding data copy, can we use ScopedVector instead? It'll make WebRtcLogUploadDoneData not copy-able, which will make it not working with std::map, i.e. UploadDoneDataMap. I don't see the benefits of doing that, considering the added complexity. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:110: const std::vector<WebRtcRtpDumpDescription>& rtp_dumps, On 2014/05/17 10:18:37, tommi wrote: > ScopedVector? See above. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:552: if (!rtp_dump_handler_) On 2014/05/17 10:18:37, tommi wrote: > if this is already non-NULL, is that something unexpected? It's not unexpected. Added comment to explain. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:572: WebRtcLogUploadDoneData upload_done_data; On 2014/05/17 10:18:37, tommi wrote: > can WebRtcLogUploadDoneData be a ScopedVector? See my previous reply. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:18: void DeleteFileHelper(const base::FilePath& path) { On 2014/05/17 10:18:37, tommi wrote: > nit: is this helper needed? (can we bind to DeleteFile directly? there's a > helper in base to ignore the return value if that's the problem) Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; On 2014/05/17 10:18:37, tommi wrote: > {} Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:284: g_ongoing_rtp_dumps--; On 2014/05/17 10:18:37, tommi wrote: > nit: --g_ongoing_rtp_dumps; Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:32: base::FilePath incoming_dump_path; On 2014/05/17 10:18:37, tommi wrote: > can these be const? Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:43: virtual ~WebRtcRtpDumpHandler(); On 2014/05/17 10:18:37, tommi wrote: > why virtual? Removed. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc:88: EXPECT_GT(base::WriteFile(*incoming_dump, dummy, arraysize(dummy)), 0); On 2014/05/17 10:18:37, tommi wrote: > is writing the terminating \0 intentional? The content of the file does not matter. It's only called to make the file exist on disk. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:18: struct RtpDumpFileHeader { On 2014/05/17 10:18:37, tommi wrote: > is there a certain packing that's assumed for these structs? If so, what is it? > Is it safe to assume that all build configurations will be configured for that > exact packing or should we be explicit? > > Maybe it would be good to have compile time assert for the sizes of these > structs? Make it independent of the packing by replacing sizeof(...) with a const. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:50: uint32 start_sec; // start of recording, the seconds part. On 2014/05/17 10:18:37, tommi wrote: > could these members all be const? (sorry, have to ask) Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:85: uint16 packet_dump_length; On 2014/05/17 10:18:37, tommi wrote: > and these ones? Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:101: for (size_t i = 0; i < src_len; ++i) { On 2014/05/17 10:18:37, tommi wrote: > nit: no {} Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:190: bool Compress(std::vector<uint8>* input, std::vector<uint8>* output) { On 2014/05/17 10:18:37, tommi wrote: > why is |input| writable? the zlib methods require z_stream::next_in to be writable. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:287: incoming_file_thread_worker_.reset(); On 2014/05/17 10:18:37, tommi wrote: > if we get here, is that an error or expected case? Can you document what will > have happened if we hit here? > > If this should never happen, I'd prefer a check here and just pass the return > value of relese() to DeleteSoon(). Same below. Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:62: size_t max_dump_size() const { return max_dump_size_; } On 2014/05/17 10:18:37, tommi wrote: > missing dcheck for IO thread? Maybe this should just be implemented in the cc > file then. Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc (right): https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:83: return true; On 2014/05/17 10:18:37, tommi wrote: > nit: this always returns true... just make it return void since it encapsulates > all the expectations? Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:182: dump_pos += 2; On 2014/05/17 10:18:37, tommi wrote: > += sizeof(uint16); ? Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:190: dump_pos += 2; On 2014/05/17 10:18:37, tommi wrote: > same here Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:193: dump_pos += 4; On 2014/05/17 10:18:37, tommi wrote: > size of the time field? Done. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:229: { On 2014/05/17 10:18:37, tommi wrote: > what is the scope for? Added comments. https://codereview.chromium.org/264793017/diff/270001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer_unittest.cc:254: { On 2014/05/17 10:18:37, tommi wrote: > same here and throughout (I'm sure I'm missing something, but if I'm not seeing > it, it might be worth documenting) added comments.
PTAL.
https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:510: packet_dump_callback_.Reset(); On 2014/05/16 22:52:43, jiayl wrote: > On 2014/05/16 22:03:22, jam wrote: > > actually, how is this method thread safe to call per the RPH document? it > > doesn't appear to be > > It's essentially WebRtcLoggingHandlerHost::OnRtpPacket, which always posts to IO > thread and WebRtcLoggingHandlerHost is thread safe ref counted. So I think it's > OK call it from any thread. I'm not sure what you mean? This callback member variable could b reset on any thread; however it's being used in the method below on the IO thread. how is that thread safe?
PTAL https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... File content/browser/renderer_host/p2p/socket_host.cc (right): https://codereview.chromium.org/264793017/diff/260001/content/browser/rendere... content/browser/renderer_host/p2p/socket_host.cc:510: packet_dump_callback_.Reset(); On 2014/05/20 15:13:14, jam wrote: > On 2014/05/16 22:52:43, jiayl wrote: > > On 2014/05/16 22:03:22, jam wrote: > > > actually, how is this method thread safe to call per the RPH document? it > > > doesn't appear to be > > > > It's essentially WebRtcLoggingHandlerHost::OnRtpPacket, which always posts to > IO > > thread and WebRtcLoggingHandlerHost is thread safe ref counted. So I think > it's > > OK call it from any thread. > > I'm not sure what you mean? This callback member variable could b reset on any > thread; however it's being used in the method below on the IO thread. how is > that thread safe? Ah, I see what you mean now. You are right, it was a problem. Now fixed by calling the callback on the IO thread with a WeakPtr of P2PSocketHost. Updated the interface documentation for RenderProcessHost as well.
https://codereview.chromium.org/264793017/diff/330001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/330001/content/public/browser/... content/public/browser/render_process_host.h:220: // |packet_callback| must be called on the IO thread. given that RPH is an object that lives on the UI thread, it's confusing and would be error prone that packet_callback and the returned callback are called on the IO thread. please make them all UI thread.
PTAL https://codereview.chromium.org/264793017/diff/330001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/330001/content/public/browser/... content/public/browser/render_process_host.h:220: // |packet_callback| must be called on the IO thread. On 2014/05/21 16:36:47, jam wrote: > given that RPH is an object that lives on the UI thread, it's confusing and > would be error prone that packet_callback and the returned callback are called > on the IO thread. please make them all UI thread. Done.
lgtm https://codereview.chromium.org/264793017/diff/350001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/264793017/diff/350001/content/public/browser/... content/public/browser/render_process_host.h:220: // on the UI thread too. nit: remove all mentions of UI thread in the comment, since by default any object in content/public runs on UI thread unless otherwise stted
Ping Henrik and tommi.
Another round. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:108: // TODO(jiayl): adds the RTP dump records to chrome://webrtc-logs. "Add the"... https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; // a user friendly name of the dump. Start sentence in comments with capital letter. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:124: // WebRtcRtpDumpWriter does not support changing the dump path after it's That can be a source of confusion in the future. I'd like the filename to reflect the actual start timestamp. That should be easily supported in the writer, shouldn't it? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:182: DVLOG(2) << "ReleaseDumps called in invalid state: incoming_state = " Should this happen? If not, dcheck. It seems like we just keep on as normal even if we're in a bad state, which seems to liberal to me. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; IS this expected to happen? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:223: ++g_ongoing_rtp_dumps; Why increase the count when assigning a writer? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:237: StopDump(type, GenericDoneCallback()); The writer calls this object, which calls back into the writer. Will that be a problem? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:263: if (DumpTypeContainsOutgoing(ended_type)) { I really, really would like to get rid of the incoming/outgoing duplication in the handler and writer. (I.e. use two handlers instead.) I can't dig up the comment with the reason not to, can you refresh me? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:18: struct RtpDumpFileHeader { This is more than a data container, make it a class. BTW, what's the reason for not just having a function? Is the data passed around or used in multiple places? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:60: struct PacketDumpHeader { Same here. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:102: for (size_t i = 0; i < src_len; ++i) This should be optimizable. Use resize and pointer to copy the data? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:25: // - WebRtcRtpDumpWriter does not stop writing to the dump after the max size Above it says it drops the packet, which is contradictory. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:35: static size_t GetRtpHeaderLen(const uint8* packet, size_t length); Is this used from outside or can it be an ordinary function in the impl file? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:37: // |dump_path| is the file path of the compressed dump file. |max_dump_size| There's no |dump_path|. :) Add incoming_ and outgoing_.
PTAL. Thanks! https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:108: // TODO(jiayl): adds the RTP dump records to chrome://webrtc-logs. On 2014/05/23 12:17:51, Henrik Grunell wrote: > "Add the"... Done. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:33: std::string name; // a user friendly name of the dump. On 2014/05/23 12:17:51, Henrik Grunell wrote: > Start sentence in comments with capital letter. Done. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:124: // WebRtcRtpDumpWriter does not support changing the dump path after it's On 2014/05/23 12:17:51, Henrik Grunell wrote: > That can be a source of confusion in the future. I'd like the filename to > reflect the actual start timestamp. That should be easily supported in the > writer, shouldn't it? The timestamp is not intended to be human friendly and only added to make each dump name unique. One should use the file creation time or last modified time to find a dump, given the time of a call. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:182: DVLOG(2) << "ReleaseDumps called in invalid state: incoming_state = " On 2014/05/23 12:17:51, Henrik Grunell wrote: > Should this happen? If not, dcheck. It seems like we just keep on as normal even > if we're in a bad state, which seems to liberal to me. Changed so the caller can call ReadyToRelease to decide if it's valid to call ReleaseDumps. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; On 2014/05/23 12:17:51, Henrik Grunell wrote: > IS this expected to happen? I'd like to be defensive for the input to the public interfaces, to save the caller from tracking the state of the dump. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:223: ++g_ongoing_rtp_dumps; On 2014/05/23 12:17:51, Henrik Grunell wrote: > Why increase the count when assigning a writer? Because the goal is to limit the total disk usage and the max dump size is applied on a writer, so it's essentially to limit the number of writers. Increasing the count here so we can test the max writer limit in unit tests. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:237: StopDump(type, GenericDoneCallback()); On 2014/05/23 12:17:51, Henrik Grunell wrote: > The writer calls this object, which calls back into the writer. Will that be a > problem? I don't think so. Writer::EndDump can be called at any time. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:263: if (DumpTypeContainsOutgoing(ended_type)) { On 2014/05/23 12:17:51, Henrik Grunell wrote: > I really, really would like to get rid of the incoming/outgoing duplication in > the handler and writer. (I.e. use two handlers instead.) I can't dig up the > comment with the reason not to, can you refresh me? Here is the previous discussion: https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:120: scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; On 2014/05/14 16:07:58, jiayl wrote: > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > So there's one writer that for both incoming and outgoing. It seems that > > incoming and outgoing can be handled completely separately, maybe there can be > > two dump handlers, one for incoming and one for outgoing? We'll get rid of all > > incoming_* and outgoing_*. Could they have a dump writer each? Or is it > tightly > > coupled together? Maybe share an injected writer? Lifetime must be ensured in > > that case. > > It'll be more difficult to apply the max dump limit if the writers are > separated, because > the limit is on the total size of all dumps. I see. And the incoming and outgoing can't be expected to be ~equal. OK, let's keep it this way. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:18: struct RtpDumpFileHeader { On 2014/05/23 12:17:51, Henrik Grunell wrote: > This is more than a data container, make it a class. BTW, what's the reason for > not just having a function? Is the data passed around or used in multiple > places? Good point. Changed to helper functions. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:60: struct PacketDumpHeader { On 2014/05/23 12:17:51, Henrik Grunell wrote: > Same here. Done. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:102: for (size_t i = 0; i < src_len; ++i) On 2014/05/23 12:17:51, Henrik Grunell wrote: > This should be optimizable. Use resize and pointer to copy the data? Done. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.h (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:25: // - WebRtcRtpDumpWriter does not stop writing to the dump after the max size On 2014/05/23 12:17:51, Henrik Grunell wrote: > Above it says it drops the packet, which is contradictory. Done. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:35: static size_t GetRtpHeaderLen(const uint8* packet, size_t length); On 2014/05/23 12:17:51, Henrik Grunell wrote: > Is this used from outside or can it be an ordinary function in the impl file? It's dead code. removed. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.h:37: // |dump_path| is the file path of the compressed dump file. |max_dump_size| On 2014/05/23 12:17:51, Henrik Grunell wrote: > There's no |dump_path|. :) Add incoming_ and outgoing_. Done.
https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; On 2014/05/23 17:48:36, jiayl wrote: > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > IS this expected to happen? > > I'd like to be defensive for the input to the public interfaces, to save the > caller from tracking the state of the dump. Is there a border case, e.g. there's a delay when starting or stopping the dump, when this is considered normal? I want to prevent programming errors where the user thinks the dump is started but it's actually not. I don't think being defensive in general is strong enough argument, I'd like to have a specific case when this is OK to happen. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:223: ++g_ongoing_rtp_dumps; On 2014/05/23 17:48:36, jiayl wrote: > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > Why increase the count when assigning a writer? > > Because the goal is to limit the total disk usage and the max dump size is > applied on a writer, so it's essentially to limit the number of writers. > Increasing the count here so we can test the max writer limit in unit tests. So, isn't start called after this? And increased again? https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:263: if (DumpTypeContainsOutgoing(ended_type)) { On 2014/05/23 17:48:36, jiayl wrote: > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > I really, really would like to get rid of the incoming/outgoing duplication in > > the handler and writer. (I.e. use two handlers instead.) I can't dig up the > > comment with the reason not to, can you refresh me? > > > Here is the previous discussion: > > https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... > chrome/browser/media/webrtc_rtp_dump_handler.h:120: > scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; > On 2014/05/14 16:07:58, jiayl wrote: > > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > > So there's one writer that for both incoming and outgoing. It seems that > > > incoming and outgoing can be handled completely separately, maybe there can > be > > > two dump handlers, one for incoming and one for outgoing? We'll get rid of > all > > > incoming_* and outgoing_*. Could they have a dump writer each? Or is it > > tightly > > > coupled together? Maybe share an injected writer? Lifetime must be ensured > in > > > that case. > > > > It'll be more difficult to apply the max dump limit if the writers are > > separated, because > > the limit is on the total size of all dumps. > > I see. And the incoming and outgoing can't be expected to be ~equal. OK, let's > keep it this way. We're just storing packet headers, right? So in what span can the header size be? Could we simply have two writers & handlers and limit the size in each to half of the current limit. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:341: profile_->GetPath())); This is not the correct log path. Use WebRtcLogList::GetWebRtcLogDirectoryForProfile(profile_->GetPath()). https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:17: static const size_t kMinimumGzipOutputBufferSize = 256; Why is this needed? What's the value based on? https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:26: output->resize(output->size() + kRtpDumpFileHeaderSize); Adding the sizes written below gives 18 bytes. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:83: Nit: Remove blank line. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:99: ~FileThreadWorker() { DCHECK(thread_checker_.CalledOnValidThread()); } Put DCHECK on its own line. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:114: if (buffer->size()) { buffer->size() > 0 When would this function be called with empty buffer? The logic in this function is a bit hard to understand, make it clearer. Or if not possible, add comments. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:116: } else if (!base::PathExists(dump_path_)) { I don't understand this if block. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:145: if (base::PathExists(dump_path_)) { I think the user of this class should ensure that the path exists and we dcheck here. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:175: if (!stream_initialized_) { Can you avoid using a flag and doing init in this function? https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:194: result = deflate(&stream_, Z_SYNC_FLUSH); Nit: AFAIK, you shouldn't trust it completing all the data in one call, but loop. In practice, I'm sure this will be fine. You can keep it like this if you're confident it's be OK. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:237: return bytes_written > 0; Will the last deflate calls for sure render output data? https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:289: if (!dest_buffer->capacity()) { Check empty() instead? This is some kind of init done once. I think it should be moved out from here and not called in this function.
PTAL https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:211: return; On 2014/05/26 09:51:03, Henrik Grunell wrote: > On 2014/05/23 17:48:36, jiayl wrote: > > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > > IS this expected to happen? > > > > I'd like to be defensive for the input to the public interfaces, to save the > > caller from tracking the state of the dump. > > Is there a border case, e.g. there's a delay when starting or stopping the dump, > when this is considered normal? > > I want to prevent programming errors where the user thinks the dump is started > but it's actually not. I don't think being defensive in general is strong enough > argument, I'd like to have a specific case when this is OK to happen. There is a delay in stopping the P2PSocketHost: the call is posted to UI thread first then to the IO thread to flip the states. So this may be called after WebRtcRtpDumpHandler is stopped but P2PSocketHost is not. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:223: ++g_ongoing_rtp_dumps; On 2014/05/26 09:51:03, Henrik Grunell wrote: > On 2014/05/23 17:48:36, jiayl wrote: > > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > > Why increase the count when assigning a writer? > > > > Because the goal is to limit the total disk usage and the max dump size is > > applied on a writer, so it's essentially to limit the number of writers. > > Increasing the count here so we can test the max writer limit in unit tests. > > So, isn't start called after this? And increased again? It's not increased again because Start only increase it when a new writer is created. https://codereview.chromium.org/264793017/diff/290001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:263: if (DumpTypeContainsOutgoing(ended_type)) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > On 2014/05/23 17:48:36, jiayl wrote: > > On 2014/05/23 12:17:51, Henrik Grunell wrote: > > > I really, really would like to get rid of the incoming/outgoing duplication > in > > > the handler and writer. (I.e. use two handlers instead.) I can't dig up the > > > comment with the reason not to, can you refresh me? > > > > > > Here is the previous discussion: > > > > > https://codereview.chromium.org/264793017/diff/150001/chrome/browser/media/we... > > chrome/browser/media/webrtc_rtp_dump_handler.h:120: > > scoped_ptr<WebRtcRtpDumpWriter> dump_writer_; > > On 2014/05/14 16:07:58, jiayl wrote: > > > On 2014/05/14 12:14:12, Henrik Grunell wrote: > > > > So there's one writer that for both incoming and outgoing. It seems that > > > > incoming and outgoing can be handled completely separately, maybe there > can > > be > > > > two dump handlers, one for incoming and one for outgoing? We'll get rid of > > all > > > > incoming_* and outgoing_*. Could they have a dump writer each? Or is it > > > tightly > > > > coupled together? Maybe share an injected writer? Lifetime must be ensured > > in > > > > that case. > > > > > > It'll be more difficult to apply the max dump limit if the writers are > > > separated, because > > > the limit is on the total size of all dumps. > > > > I see. And the incoming and outgoing can't be expected to be ~equal. OK, let's > > keep it this way. > > We're just storing packet headers, right? So in what span can the header size > be? Could we simply have two writers & handlers and limit the size in each to > half of the current limit. Incoming and outgoing can be very asymmetric. E.g. a call may have outgoing traffic only; or one direction has only video or audio, which impacts the size a lot; there may be far more incoming streams than outgoing streams; JS may only start one type of dump. Limiting the size to half the limit is sub-optimal in these cases. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:341: profile_->GetPath())); On 2014/05/26 09:51:03, Henrik Grunell wrote: > This is not the correct log path. Use > WebRtcLogList::GetWebRtcLogDirectoryForProfile(profile_->GetPath()). Done. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:17: static const size_t kMinimumGzipOutputBufferSize = 256; On 2014/05/26 09:51:03, Henrik Grunell wrote: > Why is this needed? What's the value based on? Usually we reserve the output buffer of Deflate in the same size of the input buffer. But if the input is very small, the required output buffer may be larger than the input due to the gzip header. This is the minimum output buffer size to make sure we have enough output space in all cases. The value is kind of arbitrary (based on a recommended 150B file size for gzip according to Google Search), but works in practice. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:26: output->resize(output->size() + kRtpDumpFileHeaderSize); On 2014/05/26 09:51:03, Henrik Grunell wrote: > Adding the sizes written below gives 18 bytes. Fixed: port should be uint16. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:83: On 2014/05/26 09:51:03, Henrik Grunell wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:99: ~FileThreadWorker() { DCHECK(thread_checker_.CalledOnValidThread()); } On 2014/05/26 09:51:03, Henrik Grunell wrote: > Put DCHECK on its own line. Done. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:114: if (buffer->size()) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > buffer->size() > 0 > > When would this function be called with empty buffer? The logic in this function > is a bit hard to understand, make it clearer. Or if not possible, add comments. When there is no RTP traffic since the last flush. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:116: } else if (!base::PathExists(dump_path_)) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > I don't understand this if block. Comments added. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:145: if (base::PathExists(dump_path_)) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > I think the user of this class should ensure that the path exists and we dcheck > here. The caller of the class is not running on the FILE thread, but on the IO thread. So it's much more convenient to do the check here. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:175: if (!stream_initialized_) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > Can you avoid using a flag and doing init in this function? Done. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:194: result = deflate(&stream_, Z_SYNC_FLUSH); On 2014/05/26 09:51:03, Henrik Grunell wrote: > Nit: AFAIK, you shouldn't trust it completing all the data in one call, but > loop. In practice, I'm sure this will be fine. You can keep it like this if > you're confident it's be OK. I believe it's fine since we provide enough output buffer. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:237: return bytes_written > 0; On 2014/05/26 09:51:03, Henrik Grunell wrote: > Will the last deflate calls for sure render output data? I think so. Added a DCHECK. https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:289: if (!dest_buffer->capacity()) { On 2014/05/26 09:51:03, Henrik Grunell wrote: > Check empty() instead? > > This is some kind of init done once. I think it should be moved out from here > and not called in this function. It could be empty but initialized (i.e. file header written) if no rtp packet since the last flush. It can only be initialized here because we don't want to write the file header if there is no rtp packet ever.
https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:114: if (buffer->size()) { On 2014/05/27 20:41:41, jiayl wrote: > On 2014/05/26 09:51:03, Henrik Grunell wrote: > > buffer->size() > 0 > > > > When would this function be called with empty buffer? The logic in this > function > > is a bit hard to understand, make it clearer. Or if not possible, add > comments. > > > When there is no RTP traffic since the last flush. Why will it be called if there's no data to add? https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:125: DCHECK(buffer->size() || end_stream); Since size() returns and int, check > 0. Same below. It's more readable.
https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/400001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:114: if (buffer->size()) { On 2014/05/28 11:57:51, Henrik Grunell wrote: > On 2014/05/27 20:41:41, jiayl wrote: > > On 2014/05/26 09:51:03, Henrik Grunell wrote: > > > buffer->size() > 0 > > > > > > When would this function be called with empty buffer? The logic in this > > function > > > is a bit hard to understand, make it clearer. Or if not possible, add > > comments. > > > > > > When there is no RTP traffic since the last flush. > > Why will it be called if there's no data to add? To end the gzip stream. See the new DCHECK added.
Thanks for explaining all the questions. LGTM if final comment is fixed.
vrk, could you review the */media/* changes as the MEDIA owner? tommi is OK with another MEDIA owner taking it over. Thanks!
lgtm media OWNERS Just a bunch of nits. I didn't look super carefully at everything since grunell already did a thorough pass. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:65: AddMultipartFileContentHeader(post_data, name, name + ".gz"); nit: I'd suggest you remove the |file_name| parameter and append the ".gz" in AddMultipartFileContentHeader, since AddMultipartFileContentHeader always uses Content-Type: application/gzip. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.cc:272: AddRtpDumpData(post_data, it->name, dump_data); Instead of defining "rtpdump_recv" in WebRtcLoggingHandlerHost, I think you should create kIncomingRtpDumpName and kOutgoingRtpDumpName, so you don't have to worry about things like if the caller puts a space in the name, or uses the same name twice. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_log_uploader.h (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_log_uploader.h:44: std::vector<WebRtcRtpDumpDescription> rtp_dumps; I think it'd be clearer to just have base::FilePath incoming_dump_path; base::FilePath outgoing_dump_path; and get rid of the WebRtcRtpDumpDescription struct. See other comment in webrtc_log_uploader about "name" field. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:246: stop_rtp_dump_callback_ = stop_callback; Add dcheck to make sure this is the same for incoming/outgoing. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:317: if (rtp_dump_handler_) { Add comment explaining when it is null. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:339: // The log directory has been ensured existing if |rtp_dump_handler_| is nit: This is pretty subtle. I would just keep the code as it was with the unnecessary thread hop for the rtp dump case. (The thread hop is possibly unnecessary for the normal native log case too) https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:553: DoUploadLogAndDumps(log_directory, Inline this method https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_logging_handler_host.cc:607: WebRtcRtpDumpDescription("rtpdump_recv", rtp_dumps.incoming_dump_path)); I think you should define these names in webrtc_log_uploader, see other comments. https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.cc:97: *error_message = "RTP dump already started."; Add type to the error message, like "RTP dump already started for RtpDumpType: " https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler.h (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:112: // The directory containing the dump files. Clarify: "The absolute path to the directory containing the dump files." https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler.h:115: // The dump file paths. Clarify: "The absolute path to the file containing the incoming/outgoing dump." https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_handler_unittest.cc:19: explicit FakeDumpWriter(size_t max_dump_size, nit: delete "explicit" https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... File chrome/browser/media/webrtc_rtp_dump_writer.cc (right): https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:17: static const size_t kMinimumGzipOutputBufferSize = 256; nit: comment with units https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:20: static const size_t kRtpDumpFileHeaderSize = 16; nit: Add comments, include units https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:52: // The header size for each packet dump. nit: add units https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:140: if (end_stream) { nit: if (end_stream && !EndDumpFile()) https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:266: if (!BrowserThread::DeleteSoon(BrowserThread::FILE, nit: clearer to say: bool success = BrowserThread::DeleteSoon(...) DCHECK(success) Here and below https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:320: DCHECK((type == RTP_DUMP_BOTH || type == RTP_DUMP_INCOMING) Here's a "simplification": DCHECK(type == RTP_DUMP_OUTGOING || incoming_file_thread_worker_ != NULL); DCHECK(type == RTP_DUMP_INCOMING || outgoing_file_thread_worker_ != NULL); https://codereview.chromium.org/264793017/diff/420001/chrome/browser/media/we... chrome/browser/media/webrtc_rtp_dump_writer.cc:402: if (!BrowserThread::DeleteSoon( Same comment here as above: bool success = BrowserThread::DeleteSoon(...) DCHECK(success)
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/264793017/480001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: tommi@chromium.org. Please make sure to get positive review before another CQ attempt.
lgtm
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/264793017/500001
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/264793017/520001
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/264793017/540001
Message was sent while issue was closed.
Change committed as 273745
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/264793017/560001
Message was sent while issue was closed.
Change committed as 273980 |
