Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1374233002:
Collecting encode_time_ms for each frame (Closed)
|
Created:
5 years, 2 months ago by ivica Modified:
5 years, 2 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, andresp, peah-webrtc, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCollecting encode_time_ms for each frame.
Also, in Sample struct, replacing double with the original type.
It makes more sense to save the original data as truthful as possible, and then
convert it to double later if necessary (in the plot script).
Committed: https://crrev.com/092b13384e57b33e2003d9736dfa1f491e76f938
Cr-Commit-Position: refs/heads/master@{#10184}
Patch Set 1 #Patch Set 2 : Replacing std::function with rtc::Callback2 #Patch Set 3 : Switching to C-style callback, because mac... #Patch Set 4 : Editing comments #
Total comments: 6
Patch Set 5 : Replacing static callback with an interface #Patch Set 6 : Updating a comment #
Total comments: 10
Patch Set 7 : Changing the interface name #Patch Set 8 : Again changing the name #
Total comments: 6
Patch Set 9 : Addressing comments #
Total comments: 4
Patch Set 10 : Addressing comments + rebase master #Patch Set 11 : Don't need that include in the end #Patch Set 12 : Fixing the unit test #
Total comments: 7
Patch Set 13 : Addressing comments #
Messages
Total messages: 41 (13 generated)
ivica@webrtc.org changed reviewers: + pbos@webrtc.org, sprang@webrtc.org
Unfortunately, std::function solution doesn't compile on Mac, so I have to use the old style callbacks...
https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... File webrtc/video/full_stack_plot.py (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... webrtc/video/full_stack_plot.py:37: INPUT_TIME = 1 # ms (timestamp) INPUT_TIMESTAMP .. for this and the rest as well? Esp. if you need to explain it in the comment https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... File webrtc/video/video_capture_input.cc (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... webrtc/video/video_capture_input.cc:160: frame_encoded_callback_(frame_encoded_callback_external_, No static callback functions ever please (racy and unexpectedly shared between instances). If you need this, supply a callback through the constructor, and make it a pure-virtual interface instead of C-style function pointers. I think you should provide an implementation pointer in VideoReceiveStream::Config and call ThatInterface::OnEncodedFrame(deliver_frame, encode_time_ms) or something.
https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... File webrtc/video/full_stack_plot.py (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... webrtc/video/full_stack_plot.py:37: INPUT_TIME = 1 # ms (timestamp) On 2015/09/30 09:58:57, pbos-webrtc wrote: > INPUT_TIMESTAMP > > .. for this and the rest as well? Esp. if you need to explain it in the comment This constants should use the same names VideoAnalyzer produces, as otherwise they might be confusing. I can maybe remove the new comments, and simply change encode_time_ms to encoding_time_ms (ENCODE_TIME to ENCODING_TIME), which would make clear it's not a timestamp, but probably a time interval. (encode_time_ms is a new field I added, so it's not a problem if I rename it) https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... File webrtc/video/video_capture_input.cc (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... webrtc/video/video_capture_input.cc:160: frame_encoded_callback_(frame_encoded_callback_external_, On 2015/09/30 09:58:57, pbos-webrtc wrote: > No static callback functions ever please (racy and unexpectedly shared between > instances). If you need this, supply a callback through the constructor, and > make it a pure-virtual interface instead of C-style function pointers. I think > you should provide an implementation pointer in VideoReceiveStream::Config and > call ThatInterface::OnEncodedFrame(deliver_frame, encode_time_ms) or something. Okay, will update.
https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... File webrtc/video/full_stack_plot.py (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/full_stack_p... webrtc/video/full_stack_plot.py:37: INPUT_TIME = 1 # ms (timestamp) Didn't change the name of encode_time field in the end. I still think the names shouldn't be changed. The comments are there for clarification. https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... File webrtc/video/video_capture_input.cc (right): https://codereview.webrtc.org/1374233002/diff/60001/webrtc/video/video_captur... webrtc/video/video_capture_input.cc:160: frame_encoded_callback_(frame_encoded_callback_external_, On 2015/09/30 10:45:58, ivica wrote: > On 2015/09/30 09:58:57, pbos-webrtc wrote: > > No static callback functions ever please (racy and unexpectedly shared between > > instances). If you need this, supply a callback through the constructor, and > > make it a pure-virtual interface instead of C-style function pointers. I think > > you should provide an implementation pointer in VideoReceiveStream::Config and > > call ThatInterface::OnEncodedFrame(deliver_frame, encode_time_ms) or > something. > > Okay, will update. Updated, please check if the implementation is fine.
https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... File webrtc/video/video_capture_input.cc (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.cc:156: encode_time_callback_->OnFrameEncoded(deliver_frame, encode_time_ms); OnEncodedFrame to match stats_proxy_ above https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... File webrtc/video/video_capture_input.h (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.h:51: class FrameEncodeTimeCallback { EncoderObserver or something more general (you also observe the encoded frame), and move this class to webrtc/video_send_stream.h. https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.h:55: virtual void OnFrameEncoded(const VideoFrame& frame, int encoding_time) = 0; encoding_time_ms https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream... webrtc/video_send_stream.h:159: FrameEncodeTimeCallback* encode_time_callback; = nullptr;
https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream... webrtc/video_send_stream.h:158: // 'nullptr' disables the callback. I don't think you need this last line.
https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... File webrtc/video/video_capture_input.cc (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.cc:156: encode_time_callback_->OnFrameEncoded(deliver_frame, encode_time_ms); On 2015/09/30 11:52:43, pbos-webrtc wrote: > OnEncodedFrame to match stats_proxy_ above Done. https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... File webrtc/video/video_capture_input.h (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.h:51: class FrameEncodeTimeCallback { On 2015/09/30 11:52:43, pbos-webrtc wrote: > EncoderObserver or something more general (you also observe the encoded frame), > and move this class to webrtc/video_send_stream.h. Changed to EncoderTimingObserver. https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video/video_captu... webrtc/video/video_capture_input.h:55: virtual void OnFrameEncoded(const VideoFrame& frame, int encoding_time) = 0; On 2015/09/30 11:52:43, pbos-webrtc wrote: > encoding_time_ms Changed to encode_time_ms. https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream... webrtc/video_send_stream.h:158: // 'nullptr' disables the callback. On 2015/09/30 11:53:06, pbos-webrtc wrote: > I don't think you need this last line. Removed. https://codereview.webrtc.org/1374233002/diff/100001/webrtc/video_send_stream... webrtc/video_send_stream.h:159: FrameEncodeTimeCallback* encode_time_callback; On 2015/09/30 11:52:43, pbos-webrtc wrote: > = nullptr; Whoops, fixed.
lgtm, but please let sprang do a review as well :) https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:55: : input_(nullptr), When is this set/used? https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:128: void OnEncodedFrame(const VideoFrame& frame, int encode_time_ms) { override, and imo no need for the comment https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video_send_stream... webrtc/video_send_stream.h:164: // Called for each encoded frame. Passes the total time spent on encoding. // TODO(ivica): Consolidate with post_encode_callback. Link to bug.
https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:55: : input_(nullptr), On 2015/09/30 12:48:31, pbos-webrtc wrote: > When is this set/used? Line 789 on the right side. The argument input was nullptr anyway, so I removed it. https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:128: void OnEncodedFrame(const VideoFrame& frame, int encode_time_ms) { On 2015/09/30 12:48:31, pbos-webrtc wrote: > override, and imo no need for the comment VideoAnalyzer is full of random callbacks, it is really difficult to keep track of which callback is from which interface. I'd like to keep it if that's fine with you. https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/140001/webrtc/video_send_stream... webrtc/video_send_stream.h:164: // Called for each encoded frame. Passes the total time spent on encoding. On 2015/09/30 12:48:31, pbos-webrtc wrote: > // TODO(ivica): Consolidate with post_encode_callback. Link to bug. Done.
lgtm with nits https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:526: for (size_t i = 0; i < samples_.size(); ++i) { for (const Sample& sample : samples_) https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video_send_stream.... webrtc/video_send_stream.h:26: class FrameEncodeTimeCallback; Remove.
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1374233002/#ps180001 (title: "Don't need that include in the end")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374233002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374233002/180001
https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video/video_qualit... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video/video_qualit... webrtc/video/video_quality_test.cc:526: for (size_t i = 0; i < samples_.size(); ++i) { On 2015/09/30 13:34:53, språng wrote: > for (const Sample& sample : samples_) You're right. At some point I actually needed the index, but forgot to revert that. https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/80008/webrtc/video_send_stream.... webrtc/video_send_stream.h:26: class FrameEncodeTimeCallback; On 2015/09/30 13:34:53, språng wrote: > Remove. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1018)
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1374233002/#ps200001 (title: "Fixing the unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374233002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374233002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1019)
ivica@webrtc.org changed reviewers: + mflodman@webrtc.org
Adding mflodman@ as a reviewer (he's the owner of webrtc/video_send_stream.h).
mflodman@webrtc.org changed reviewers: + asapersson@webrtc.org
+Åsa who has lloked the most at these stats.
lgtm
The CQ bit was checked by ivica@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374233002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374233002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1081)
https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:130: } override https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream... webrtc/video_send_stream.h:33: virtual void OnEncodedFrame(const VideoFrame& frame, int encode_time_ms) = 0; Not confuse this with EncodedFrameObserver::EncodedFrameCallback, I'd prefer to: 1. Replace the 'const VideoFrame& frame' argument with the capture ntp time only, since that is what's being used by the 'EncodingTimeObserver' implementation. 2. Rename 'OnEncodedFrame' to 'OnReportEncodedTime', or something similar.
https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:130: } On 2015/10/06 11:11:26, mflodman wrote: > override Hmm, it already has override. https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream... webrtc/video_send_stream.h:33: virtual void OnEncodedFrame(const VideoFrame& frame, int encode_time_ms) = 0; On 2015/10/06 11:11:27, mflodman wrote: > Not confuse this with EncodedFrameObserver::EncodedFrameCallback, I'd prefer to: > 1. Replace the 'const VideoFrame& frame' argument with the capture ntp time > only, since that is what's being used by the 'EncodingTimeObserver' > implementation. > 2. Rename 'OnEncodedFrame' to 'OnReportEncodedTime', or something similar. Makes sense, this was really confusing. Renamed.
lgtm https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:130: } On 2015/10/06 12:04:32, ivica wrote: > On 2015/10/06 11:11:26, mflodman wrote: > > override > > Hmm, it already has override. Doh, the window was too small and I missed that. ***feeling embarrassed*** https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream.h File webrtc/video_send_stream.h (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video_send_stream... webrtc/video_send_stream.h:33: virtual void OnEncodedFrame(const VideoFrame& frame, int encode_time_ms) = 0; On 2015/10/06 12:04:32, ivica wrote: > On 2015/10/06 11:11:27, mflodman wrote: > > Not confuse this with EncodedFrameObserver::EncodedFrameCallback, I'd prefer > to: > > 1. Replace the 'const VideoFrame& frame' argument with the capture ntp time > > only, since that is what's being used by the 'EncodingTimeObserver' > > implementation. > > 2. Rename 'OnEncodedFrame' to 'OnReportEncodedTime', or something similar. > > Makes sense, this was really confusing. Renamed. Great!
The CQ bit was checked by ivica@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org, pbos@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1374233002/#ps220001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374233002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374233002/220001
https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/1374233002/diff/200001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:130: } On 2015/10/06 12:10:00, mflodman wrote: > On 2015/10/06 12:04:32, ivica wrote: > > On 2015/10/06 11:11:26, mflodman wrote: > > > override > > > > Hmm, it already has override. > > Doh, the window was too small and I missed that. ***feeling embarrassed*** Haha :)
Message was sent while issue was closed.
Committed patchset #13 (id:220001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/092b13384e57b33e2003d9736dfa1f491e76f938 Cr-Commit-Position: refs/heads/master@{#10184}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:220001) has been created in https://codereview.webrtc.org/1383283005/ by kjellander@webrtc.org. The reason for reverting is: Breaks EndToEndTest.AssignsTransportSequenceNumbers in video_engine_tests on several bots: http://build.chromium.org/p/client.webrtc/builders/Linux64%20Debug/builds/5507 http://build.chromium.org/p/client.webrtc/builders/Mac64%20Debug/builds/4815 http://build.chromium.org/p/client.webrtc/builders/Win%20SyzyASan/builds/3272 http://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/4414 It seems very unfortunate that it breaks on _exactly_ the bot configs that aren't covered by the CQ trybots.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
