Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1289933003:
Full stack graphs (Closed)
|
Created:
5 years, 4 months ago by ivica Modified:
5 years, 3 months ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, mflodman, perkj_webrtc, andresp Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFull stack graphs
Updating full stack test to optionally save metadata for each frame and save it
to a file with given filename (controlled from the new full_stack_samples
executable).
Adding a Python script that reads the output generated by full stack test
and plots the graph(s).
Committed: https://crrev.com/05cfcd34693d86de7a1a481f071eae561361588a
Cr-Commit-Position: refs/heads/master@{#9874}
Patch Set 1 #Patch Set 2 : Small fixes #Patch Set 3 : Unsigned to signed int conversion fix #Patch Set 4 : Removing unused define + rebase master #
Total comments: 16
Patch Set 5 : Addressed comments #Patch Set 6 : printf size_t + git cl format #Patch Set 7 : Fixing offset and frames arguments #Patch Set 8 : Adding option to save the plot to a file #Patch Set 9 : Fixing labels and field none. #
Total comments: 13
Patch Set 10 : Addressed comments #Patch Set 11 : Fixing docstring #
Total comments: 12
Patch Set 12 : Addressing comments #
Total comments: 2
Patch Set 13 : rebase master #Patch Set 14 : rebase master #Patch Set 15 : addressing comments #
Messages
Total messages: 49 (19 generated)
ivica@webrtc.org changed reviewers: + mflodman@webrtc.org, sprang@webrtc.org
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/377)
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/9043)
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/388)
https://codereview.webrtc.org/1289933003/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:410: vpx_codec_control(encoder_, VP8E_SET_STATIC_THRESHOLD, 1); one-line comments about what these are supposed to do would be nice https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:159: length - (header.headerLength + header.paddingLength); Rebase error? This is handles by EncodedFrameCallback() now. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:398: } Remove braces https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:478: comparison.render_time_ms); // decoded_time_ms I don't think comparison.send_stats is available here. (Handled by PollStats() since a later revision) Either remove them, or if your really need them poll send stats here. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:526: fprintf(f, "%d\n", (int)samples_.size()); Use static_cast rather than c-style cast. Or just use "%lu\n" ? https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:534: } Macros are unfortunately only allowed in some very rare circumstances. Break PRINT_SAMPLES out as method instead. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:556: bool save_samples_; Not sure this is necessary. Calling graph_data_filename_.empty() once per frame shouldn't be too much overhead. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.h... webrtc/video/full_stack.h:36: std::string graph_data_filename; Would like a more descriptive name. stats_output_filename perhaps?
https://codereview.webrtc.org/1289933003/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:410: vpx_codec_control(encoder_, VP8E_SET_STATIC_THRESHOLD, 1); On 2015/08/19 13:33:37, språng wrote: > one-line comments about what these are supposed to do would be nice Done. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:159: length - (header.headerLength + header.paddingLength); On 2015/08/19 13:33:37, språng wrote: > Rebase error? This is handles by EncodedFrameCallback() now. For graphs I need encoded frame size for each frame separately. Is there any better way to gather that information? https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:398: } On 2015/08/19 13:33:37, språng wrote: > Remove braces Done. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:478: comparison.render_time_ms); // decoded_time_ms On 2015/08/19 13:33:37, språng wrote: > I don't think comparison.send_stats is available here. (Handled by PollStats() > since a later revision) > Either remove them, or if your really need them poll send stats here. Removed. Can be added later if necessary. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:526: fprintf(f, "%d\n", (int)samples_.size()); On 2015/08/19 13:33:37, språng wrote: > Use static_cast rather than c-style cast. > Or just use "%lu\n" ? Using %" PRIuS " as described in style guide... https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:534: } On 2015/08/19 13:33:37, språng wrote: > Macros are unfortunately only allowed in some very rare circumstances. > Break PRINT_SAMPLES out as method instead. Changed from one-field-one-row to one-field-one-column format and got rid of the macro. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.c... webrtc/video/full_stack.cc:556: bool save_samples_; On 2015/08/19 13:33:37, språng wrote: > Not sure this is necessary. Calling graph_data_filename_.empty() once per frame > shouldn't be too much overhead. Removed. https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/60001/webrtc/video/full_stack.h... webrtc/video/full_stack.h:36: std::string graph_data_filename; On 2015/08/19 13:33:37, språng wrote: > Would like a more descriptive name. stats_output_filename perhaps? Done.
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/441)
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/469)
ivica@webrtc.org changed reviewers: + pbos@webrtc.org - mflodman@webrtc.org
Replacing mflodman with pbos for review.
On 2015/08/25 08:47:12, ivica wrote: > Replacing mflodman with pbos for review. you don't have to replace mflodman, he can still be there :)
ivica@webrtc.org changed reviewers: + mflodman@webrtc.org
Putting mflodman back :P
The CQ bit was checked by ivica@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:157: length - (header.headerLength + header.paddingLength); Do we ever use simulcast in these tests? Then this could be racy? For VP9 svc, this is the combined size of all spatial layers for this frame? https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:171: } Don't think you need these extra {} https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:454: comparison.render_time_ms); // decoded_time_ms Why use a temporary? https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:512: for (Sample& sample : samples_) { const Sample& https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... File webrtc/video/full_stack_samples.cc (right): https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:49: DEFINE_int32(is_screenshare, 1, "Is screenshare?"); Isn't there a DEFINE_bool? https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:107: 0.0, Comment what these two 0.0 are
Also renamed decoded_time_ms to render_time_ms, because it might include intentional delay which smooths out the output video. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:157: length - (header.headerLength + header.paddingLength); On 2015/09/03 08:59:39, språng wrote: > Do we ever use simulcast in these tests? Then this could be racy? > For VP9 svc, this is the combined size of all spatial layers for this frame? Simulcast is not supported here yet. That will be another CL. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:171: } On 2015/09/03 08:59:39, språng wrote: > Don't think you need these extra {} Done. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:454: comparison.render_time_ms); // decoded_time_ms On 2015/09/03 08:59:39, språng wrote: > Why use a temporary? Done. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:512: for (Sample& sample : samples_) { On 2015/09/03 08:59:39, språng wrote: > const Sample& Done. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... File webrtc/video/full_stack_samples.cc (right): https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:49: DEFINE_int32(is_screenshare, 1, "Is screenshare?"); On 2015/09/03 08:59:39, språng wrote: > Isn't there a DEFINE_bool? Done. https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:107: 0.0, On 2015/09/03 08:59:39, språng wrote: > Comment what these two 0.0 are Done.
lgtm pbos@ ptal https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/160001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:157: length - (header.headerLength + header.paddingLength); On 2015/09/03 09:30:34, ivica wrote: > On 2015/09/03 08:59:39, språng wrote: > > Do we ever use simulcast in these tests? Then this could be racy? > > For VP9 svc, this is the combined size of all spatial layers for this frame? > > Simulcast is not supported here yet. That will be another CL. Acknowledged.
https://codereview.webrtc.org/1289933003/diff/200001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:407: if (codec_.mode == kScreensharing) { This doesn't sound like what's in the CL description? If this should be in here, please update the CL description to include what's going on. https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:500: FILE* f = fopen(stats_output_filename_.c_str(), "w"); s/f/out/ or s/f/file/, your choice. Pref first. https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.h:36: std::string stats_output_filename; Is this the graph filename? If so put graph in this variable name https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack_... File webrtc/video/full_stack_samples.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:1: /* I don't understand this filename, what're you trying to do with this file? full_stack_quality_sampler.cc ? https://codereview.webrtc.org/1289933003/diff/200001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/webrtc_tests.gypi... webrtc/webrtc_tests.gypi:48: 'target_name': 'full_stack_samples', I don't understand this binary name, should this be something like full_stack_quality_sampler or something?
https://codereview.webrtc.org/1289933003/diff/200001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:407: if (codec_.mode == kScreensharing) { On 2015/09/03 14:59:27, pbos-webrtc wrote: > This doesn't sound like what's in the CL description? If this should be in here, > please update the CL description to include what's going on. You're right, moving it to another CL. https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:500: FILE* f = fopen(stats_output_filename_.c_str(), "w"); On 2015/09/03 14:59:27, pbos-webrtc wrote: > s/f/out/ or s/f/file/, your choice. Pref first. Done. https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.h:36: std::string stats_output_filename; On 2015/09/03 14:59:27, pbos-webrtc wrote: > Is this the graph filename? If so put graph in this variable name This is only the graph textual data, not the image itself. Should I anyway put graph_output_filename, output_graph_filename or something like that? https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack_... File webrtc/video/full_stack_samples.cc (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack_... webrtc/video/full_stack_samples.cc:1: /* On 2015/09/03 14:59:27, pbos-webrtc wrote: > I don't understand this filename, what're you trying to do with this file? > > full_stack_quality_sampler.cc ? Renamed to full_stack_quality_sampler.cc. https://codereview.webrtc.org/1289933003/diff/200001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/webrtc_tests.gypi... webrtc/webrtc_tests.gypi:48: 'target_name': 'full_stack_samples', On 2015/09/03 14:59:27, pbos-webrtc wrote: > I don't understand this binary name, should this be something like > full_stack_quality_sampler or something? Renamed to full_stack_quality_sampler.
lgtm with comments fixed https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.h:36: std::string stats_output_filename; On 2015/09/03 16:24:23, ivica wrote: > On 2015/09/03 14:59:27, pbos-webrtc wrote: > > Is this the graph filename? If so put graph in this variable name > > This is only the graph textual data, not the image itself. Should I anyway put > graph_output_filename, output_graph_filename or something like that? graph_data_output_filename then? https://codereview.webrtc.org/1289933003/diff/220001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/220001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:501: if (out == NULL) { CHECK(out != nullptr) << "Couldn't open file: " << stats_output_filename_; instead of this block
https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.h File webrtc/video/full_stack.h (right): https://codereview.webrtc.org/1289933003/diff/200001/webrtc/video/full_stack.... webrtc/video/full_stack.h:36: std::string stats_output_filename; On 2015/09/07 09:51:04, pbos-webrtc wrote: > On 2015/09/03 16:24:23, ivica wrote: > > On 2015/09/03 14:59:27, pbos-webrtc wrote: > > > Is this the graph filename? If so put graph in this variable name > > > > This is only the graph textual data, not the image itself. Should I anyway put > > graph_output_filename, output_graph_filename or something like that? > > graph_data_output_filename then? Done. https://codereview.webrtc.org/1289933003/diff/220001/webrtc/video/full_stack.cc File webrtc/video/full_stack.cc (right): https://codereview.webrtc.org/1289933003/diff/220001/webrtc/video/full_stack.... webrtc/video/full_stack.cc:501: if (out == NULL) { On 2015/09/07 09:51:05, pbos-webrtc wrote: > CHECK(out != nullptr) << "Couldn't open file: " << stats_output_filename_; > > instead of this block Done.
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/1289933003/#ps280001 (title: "addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289933003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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/1289933003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289933003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/05cfcd34693d86de7a1a481f071eae561361588a Cr-Commit-Position: refs/heads/master@{#9874} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
