Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
697463005:
Add PipelineStatus UMA logging to media_internals (Closed)
DescriptionAdd PipelineStatus UMA logging to media_internals
Listen for media_log events and save every players state in a map indexed by renderer_id.
When a tab is closed, all players corresponding to the renderer will be logged to UMA.
BUG=dalecurtis@chromium.org
Committed: https://crrev.com/53bb9182b3e0ae046f228b921f03c13200aee7fe
Cr-Commit-Position: refs/heads/master@{#303973}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Fixing CL comments #
Total comments: 27
Patch Set 3 : Fix CL comments #Patch Set 4 : Change "pipeline_error" media_log event to enum #
Total comments: 12
Patch Set 5 : Fix CL comments #
Total comments: 5
Patch Set 6 : Fix CL comments #
Total comments: 2
Patch Set 7 : Posting task to IO thread on tab close #
Total comments: 10
Patch Set 8 : Adding DCHECK for currentthread == IO #
Messages
Total messages: 25 (4 generated)
prabhur@chromium.org changed reviewers: + dalecurtis@chromium.org
PTAL. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:270: &player_info[event.id]->video_decoder); Just noticed the formatting. Will fix it in the next iteration. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:285: } else if (player_info.video_codec_name == "vp9") { This code is not getting hit while playing a vp9 video. It is because media_internals is not getting the corresponding "video_decode_name" event. Possibly a bug ? https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:315: UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.Unsupported", Youtube videos end up showing as Unsupported because a lot of media_log events are not received. Could be because those videos use MSE and chunk_demuxer doesnt currently have any media_log eventing enabled. Should the chunk demuxer do the same thing as ffmegdemuxer - w.r.t sending media_log events ?
https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:245: std::string status = ""; No need for ="" https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:285: } else if (player_info.video_codec_name == "vp9") { On 2014/11/06 21:17:51, prabhur1 wrote: > This code is not getting hit while playing a vp9 video. It is because > media_internals is not getting the corresponding "video_decode_name" event. > Possibly a bug ? Yeah sounds like it, can you fix or file a bug? https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:315: UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.Unsupported", On 2014/11/06 21:17:51, prabhur1 wrote: > Youtube videos end up showing as Unsupported because a lot of media_log events > are not received. Could be because those videos use MSE and chunk_demuxer doesnt > currently have any media_log eventing enabled. > Should the chunk demuxer do the same thing as ffmegdemuxer - w.r.t sending > media_log events ? Hmm, yeah it looks like MSE is sending the raw video codec and audio codec IDs through error strings. :| Probably we should add some code to MSE to log the codec_ids as reported to AudioDecoderConfig/VideoDecoderConfig. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:34: public NotificationObserver { Instead of adding this to the main MediaInternals class, you should just add a private inner class to the CC file that MediaInternals can own; just like AudioLogImpl is done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:74: media::PipelineStatus last_pipeline_status; If you take my advice above this whole thing should move into the .cc file. You should move all the initialization into constructor parameters. See the syntax on MediaInternals(). https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:83: audio_codec_name = ""; Not necessary to initialize the strings. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:91: typedef std::map<int, PipelineInfo*> PlayerInfoMap; Don't use a pointer, no need since you're providing a default constructor... https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:109: void LogUMAForPipelineStatus(const PipelineInfo& player_info); s/Log/Report/ https://codereview.chromium.org/697463005/diff/1/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.cc#newc... media/base/media_log.cc:103: static std::map<std::string, PipelineStatus> status_map = { Instead just create a parallel array of strings which matches the pipelinestatus codes and add a compile_assert() to ensure they stay in sync. I.e. static const char* PlayerStatus[] = { "pipeline: ok", // == PIPELINE_OK ... }; You can then just iterate and return the right status code based on position. https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h#newco... media/base/media_log.h:44: static bool StringToPipelineStatus(const std::string&, PipelineStatus&); No need for & for primitive type like status. Both parameters need names too, can you fix the one above as well?
prabhur@chromium.org changed reviewers: + asvitkine@chromium.org, isherman@chromium.org
PTAL https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:245: std::string status = ""; On 2014/11/06 23:49:08, DaleCurtis wrote: > No need for ="" Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:270: &player_info[event.id]->video_decoder); On 2014/11/06 21:17:51, prabhur1 wrote: > Just noticed the formatting. Will fix it in the next iteration. Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:285: } else if (player_info.video_codec_name == "vp9") { On 2014/11/06 23:49:08, DaleCurtis wrote: > On 2014/11/06 21:17:51, prabhur1 wrote: > > This code is not getting hit while playing a vp9 video. It is because > > media_internals is not getting the corresponding "video_decode_name" event. > > Possibly a bug ? > > Yeah sounds like it, can you fix or file a bug? Assigned to me : https://code.google.com/p/chromium/issues/detail?id=431443 Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:315: UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.Unsupported", On 2014/11/06 23:49:08, DaleCurtis wrote: > On 2014/11/06 21:17:51, prabhur1 wrote: > > Youtube videos end up showing as Unsupported because a lot of media_log events > > are not received. Could be because those videos use MSE and chunk_demuxer > doesnt > > currently have any media_log eventing enabled. > > Should the chunk demuxer do the same thing as ffmegdemuxer - w.r.t sending > > media_log events ? > > Hmm, yeah it looks like MSE is sending the raw video codec and audio codec IDs > through error strings. :| Probably we should add some code to MSE to log the > codec_ids as reported to AudioDecoderConfig/VideoDecoderConfig. Assigned to me: https://code.google.com/p/chromium/issues/detail?id=431447 https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:315: UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.Unsupported", On 2014/11/06 23:49:08, DaleCurtis wrote: > On 2014/11/06 21:17:51, prabhur1 wrote: > > Youtube videos end up showing as Unsupported because a lot of media_log events > > are not received. Could be because those videos use MSE and chunk_demuxer > doesnt > > currently have any media_log eventing enabled. > > Should the chunk demuxer do the same thing as ffmegdemuxer - w.r.t sending > > media_log events ? > > Hmm, yeah it looks like MSE is sending the raw video codec and audio codec IDs > through error strings. :| Probably we should add some code to MSE to log the > codec_ids as reported to AudioDecoderConfig/VideoDecoderConfig. Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:34: public NotificationObserver { On 2014/11/06 23:49:08, DaleCurtis wrote: > Instead of adding this to the main MediaInternals class, you should just add a > private inner class to the CC file that MediaInternals can own; just like > AudioLogImpl is done. Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:74: media::PipelineStatus last_pipeline_status; On 2014/11/06 23:49:08, DaleCurtis wrote: > If you take my advice above this whole thing should move into the .cc file. You > should move all the initialization into constructor parameters. See the syntax > on MediaInternals(). Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:83: audio_codec_name = ""; On 2014/11/06 23:49:08, DaleCurtis wrote: > Not necessary to initialize the strings. Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:91: typedef std::map<int, PipelineInfo*> PlayerInfoMap; On 2014/11/06 23:49:08, DaleCurtis wrote: > Don't use a pointer, no need since you're providing a default constructor... Done. https://codereview.chromium.org/697463005/diff/1/content/browser/media/media_... content/browser/media/media_internals.h:109: void LogUMAForPipelineStatus(const PipelineInfo& player_info); On 2014/11/06 23:49:08, DaleCurtis wrote: > s/Log/Report/ Done. https://codereview.chromium.org/697463005/diff/1/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.cc#newc... media/base/media_log.cc:103: static std::map<std::string, PipelineStatus> status_map = { On 2014/11/06 23:49:08, DaleCurtis wrote: > Instead just create a parallel array of strings which matches the pipelinestatus > codes and add a compile_assert() to ensure they stay in sync. I.e. > > static const char* PlayerStatus[] = { > "pipeline: ok", // == PIPELINE_OK > ... > }; > > You can then just iterate and return the right status code based on position. Done. https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h#newco... media/base/media_log.h:44: static bool StringToPipelineStatus(const std::string&, PipelineStatus&); On 2014/11/06 23:49:08, DaleCurtis wrote: > No need for & for primitive type like status. Both parameters need names too, > can you fix the one above as well? Done.
PTAL PS#2
histograms lgtm
Sorry this fell off my radar. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:188: MediaInternalsUMAHandler(); nit: constructor is usually first. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:212: RendererPlayerMap renderer_info; Member variables should end in an underscore. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:215: }; Add DISALLOW_COPY_AND_ASSIGN(...); https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:275: void MediaInternals::MediaInternalsUMAHandler::ReportUMAForPipelineStatus( Did git cl format produce this formatting? It seems wrong. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:323: while (it != players_it->second.end()) { Note: You're modifying the structure you're iterating over, this isn't always okay, but is when using Map like this: http://www.cplusplus.com/reference/map/map/erase/ See iterator validity. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:334: uma_handler.reset(new MediaInternalsUMAHandler()); Do this during the constructor variable initialization (like owner_ids_) https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:343: for (std::vector<media::MediaLogEvent>::const_iterator event = events.begin(); This is a prime candidate for conversion to "auto" :) https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.h:70: scoped_ptr<MediaInternalsUMAHandler> uma_handler; Needs underscore_ should be at the end of the class with the other variables.
Some more https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h#newco... media/base/media_log.h:44: static bool StringToPipelineStatus(const std::string&, PipelineStatus&); On 2014/11/07 23:43:24, prabhur1 wrote: > On 2014/11/06 23:49:08, DaleCurtis wrote: > > No need for & for primitive type like status. Both parameters need names too, > > can you fix the one above as well? > > Done. This is backwards of what I meant, std::string should be const&, while PipelineStatus should be a pointer. We don't use non-const & in functions. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:101: bool MediaLog::StringToPipelineStatus(const std::string status, Instead of having this wacky string conversion, have you considered adding a MediaInternals event for reporting PipelineStatus directly instead of using PipelineStatusToString()? https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:103: static const char* player_status[] = { Move this and compile_assert outside of the function. Rename something like kPlayerStatusMap and use it with the above function too. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:117: "dumuxer: could not parse", // == DEMUXER_ERROR_COULD_NOT_PARSE demuxer instead of dumuxer :) https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:123: for (uint32 i = 0; i < arraysize(player_status); i++) { size_t https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:125: out_status = (PipelineStatus)i; static_cast<PipelineStatus>(i)
asvitkine@chromium.org changed reviewers: - asvitkine@chromium.org
Removing myself as reviewer since Ilya already looked at histograms. By the way, your BUG= line is incorrect - it should point to a crbug number.
PTAL PS3 + PS4 https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/697463005/diff/1/media/base/media_log.h#newco... media/base/media_log.h:44: static bool StringToPipelineStatus(const std::string&, PipelineStatus&); On 2014/11/11 20:48:58, DaleCurtis wrote: > On 2014/11/07 23:43:24, prabhur1 wrote: > > On 2014/11/06 23:49:08, DaleCurtis wrote: > > > No need for & for primitive type like status. Both parameters need names > too, > > > can you fix the one above as well? > > > > Done. > > This is backwards of what I meant, std::string should be const&, while > PipelineStatus should be a pointer. We don't use non-const & in functions. Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:188: MediaInternalsUMAHandler(); On 2014/11/11 19:24:01, DaleCurtis wrote: > nit: constructor is usually first. Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:212: RendererPlayerMap renderer_info; On 2014/11/11 19:24:01, DaleCurtis wrote: > Member variables should end in an underscore. Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:215: }; On 2014/11/11 19:24:01, DaleCurtis wrote: > Add DISALLOW_COPY_AND_ASSIGN(...); Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:275: void MediaInternals::MediaInternalsUMAHandler::ReportUMAForPipelineStatus( On 2014/11/11 19:24:01, DaleCurtis wrote: > Did git cl format produce this formatting? It seems wrong. Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:323: while (it != players_it->second.end()) { On 2014/11/11 19:24:01, DaleCurtis wrote: > Note: You're modifying the structure you're iterating over, this isn't always > okay, but is when using Map like this: > > http://www.cplusplus.com/reference/map/map/erase/ > > See iterator validity. Yep. it++ takes care of advancing the iterator before deleting the current one. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:323: while (it != players_it->second.end()) { On 2014/11/11 19:24:01, DaleCurtis wrote: > Note: You're modifying the structure you're iterating over, this isn't always > okay, but is when using Map like this: > > http://www.cplusplus.com/reference/map/map/erase/ > > See iterator validity. Acknowledged. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:334: uma_handler.reset(new MediaInternalsUMAHandler()); On 2014/11/11 19:24:01, DaleCurtis wrote: > Do this during the constructor variable initialization (like owner_ids_) Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.cc:343: for (std::vector<media::MediaLogEvent>::const_iterator event = events.begin(); On 2014/11/11 19:24:01, DaleCurtis wrote: > This is a prime candidate for conversion to "auto" :) Done. https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... File content/browser/media/media_internals.h (right): https://codereview.chromium.org/697463005/diff/20001/content/browser/media/me... content/browser/media/media_internals.h:70: scoped_ptr<MediaInternalsUMAHandler> uma_handler; On 2014/11/11 19:24:01, DaleCurtis wrote: > Needs underscore_ should be at the end of the class with the other variables. Done. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:101: bool MediaLog::StringToPipelineStatus(const std::string status, On 2014/11/11 20:48:58, DaleCurtis wrote: > Instead of having this wacky string conversion, have you considered adding a > MediaInternals event for reporting PipelineStatus directly instead of using > PipelineStatusToString()? Done. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:103: static const char* player_status[] = { On 2014/11/11 20:48:58, DaleCurtis wrote: > Move this and compile_assert outside of the function. Rename something like > kPlayerStatusMap and use it with the above function too. Done. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:117: "dumuxer: could not parse", // == DEMUXER_ERROR_COULD_NOT_PARSE On 2014/11/11 20:48:58, DaleCurtis wrote: > demuxer instead of dumuxer :) Done. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:123: for (uint32 i = 0; i < arraysize(player_status); i++) { On 2014/11/11 20:48:58, DaleCurtis wrote: > size_t Done. https://codereview.chromium.org/697463005/diff/20001/media/base/media_log.cc#... media/base/media_log.cc:125: out_status = (PipelineStatus)i; On 2014/11/11 20:48:58, DaleCurtis wrote: > static_cast<PipelineStatus>(i) Done.
Looking good. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:181: // NotificationObserver implementation. Add a blank line above this. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:185: void LogAndClearPlayersInRenderer(int render_process_id); Can you add some docs for these last two methods? And separate this one from the Observe() call since it's not part of the NotificationObserver implementation. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:215: void ReportUMAForPipelineStatus(const PipelineInfo& player_info); Methods go above variable declarations. Add some comments for this too. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:363: base::DictionaryValue error_status; Why do this instead of doing event->params.SetString() ? https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:367: } else Need {} for else
PTAL PS5 https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:181: // NotificationObserver implementation. On 2014/11/12 21:41:09, DaleCurtis wrote: > Add a blank line above this. Done. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:185: void LogAndClearPlayersInRenderer(int render_process_id); On 2014/11/12 21:41:09, DaleCurtis wrote: > Can you add some docs for these last two methods? And separate this one from the > Observe() call since it's not part of the NotificationObserver implementation. Done. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:185: void LogAndClearPlayersInRenderer(int render_process_id); On 2014/11/12 21:41:09, DaleCurtis wrote: > Can you add some docs for these last two methods? And separate this one from the > Observe() call since it's not part of the NotificationObserver implementation. Did you mean adding a new line separator ? https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:215: void ReportUMAForPipelineStatus(const PipelineInfo& player_info); On 2014/11/12 21:41:08, DaleCurtis wrote: > Methods go above variable declarations. Add some comments for this too. Done. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:215: void ReportUMAForPipelineStatus(const PipelineInfo& player_info); On 2014/11/12 21:41:08, DaleCurtis wrote: > Methods go above variable declarations. Add some comments for this too. Moved after the struct because of dependency on PipelineInfo definition. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:363: base::DictionaryValue error_status; On 2014/11/12 21:41:08, DaleCurtis wrote: > Why do this instead of doing event->params.SetString() ? event is a const_iterator and doesn't allow call to SetString() as it is not a const method. https://codereview.chromium.org/697463005/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:367: } else On 2014/11/12 21:41:09, DaleCurtis wrote: > Need {} for else Done.
https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... content/browser/media/media_internals.cc:196: NotificationRegistrar registrar_; Move down lower. https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... content/browser/media/media_internals.cc:372: dict.Set("params", error_status.DeepCopy()); Does setting params.pipeline_error to the right string value work instead of doing the deep_copy? I believe these DictioaryValue types follow . syntax.
PTAL PS#6 https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... content/browser/media/media_internals.cc:196: NotificationRegistrar registrar_; On 2014/11/12 22:29:25, DaleCurtis wrote: > Move down lower. ah..sorry about that! https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... content/browser/media/media_internals.cc:372: dict.Set("params", error_status.DeepCopy()); On 2014/11/12 22:29:25, DaleCurtis wrote: > Does setting params.pipeline_error to the right string value work instead of > doing the deep_copy? I believe these DictioaryValue types follow . syntax. Yes, that works. https://codereview.chromium.org/697463005/diff/80001/content/browser/media/me... content/browser/media/media_internals.cc:372: dict.Set("params", error_status.DeepCopy()); On 2014/11/12 22:29:25, DaleCurtis wrote: > Does setting params.pipeline_error to the right string value work instead of > doing the deep_copy? I believe these DictioaryValue types follow . syntax. Done. https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:192: void SavePlayerState(const media::MediaLogEvent& event, The above two functions are called from different places (likely different threads?). Could there be a race in updating the map eg:- when media_event is received at the same time a tab is closed. Not sure how practical this is but theoretically possible. Is that a concern ? do we need a mutual exclusion lock?
https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... content/browser/media/media_internals.cc:192: void SavePlayerState(const media::MediaLogEvent& event, On 2014/11/12 23:26:33, prabhur1 wrote: > The above two functions are called from different places (likely different > threads?). Could there be a race in updating the map eg:- when media_event is > received at the same time a tab is closed. Not sure how practical this is but > theoretically possible. Is that a concern ? do we need a mutual exclusion lock? If notifications aren't delivered on the IO thread, then yes you'll need a lock.
On 2014/11/13 00:08:17, DaleCurtis wrote: > https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... > File content/browser/media/media_internals.cc (right): > > https://codereview.chromium.org/697463005/diff/100001/content/browser/media/m... > content/browser/media/media_internals.cc:192: void SavePlayerState(const > media::MediaLogEvent& event, > On 2014/11/12 23:26:33, prabhur1 wrote: > > The above two functions are called from different places (likely different > > threads?). Could there be a race in updating the map eg:- when media_event is > > received at the same time a tab is closed. Not sure how practical this is but > > theoretically possible. Is that a concern ? do we need a mutual exclusion > lock? > > If notifications aren't delivered on the IO thread, then yes you'll need a lock. I have the following in th notification handler. Since it is from the UI thread, I guess we need a lock ? Is that what you meant ? DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)
PTAL #PS7 https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:243: // it is owned by MediaInternals and share the same lifetime Could you confirm if my assumption is correct about lifetime of MediaInternalsUMAHandler & the use of base::Unretained()? I didnt see any issues/crashes in local testing.
lgtm https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:243: // it is owned by MediaInternals and share the same lifetime On 2014/11/13 01:54:39, prabhur1 wrote: > Could you confirm if my assumption is correct about lifetime of > MediaInternalsUMAHandler & the use of base::Unretained()? > I didnt see any issues/crashes in local testing. Unretained is fine since this class is never destroyed :) https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:253: PlayerInfoMap& player_info = renderer_info_[render_process_id]; Add a DCHECK(CurrentlyOn::IO) ? https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:296: if (player_info.has_video && player_info.has_audio) { Ditto. https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:337: auto players_it = renderer_info_.find(render_process_id); Ditto.
Thanks for the review Dale! Appreciate the help. Added the DCHECKs. https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:243: // it is owned by MediaInternals and share the same lifetime On 2014/11/13 01:57:48, DaleCurtis wrote: > On 2014/11/13 01:54:39, prabhur1 wrote: > > Could you confirm if my assumption is correct about lifetime of > > MediaInternalsUMAHandler & the use of base::Unretained()? > > I didnt see any issues/crashes in local testing. > > Unretained is fine since this class is never destroyed :) Done. https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:243: // it is owned by MediaInternals and share the same lifetime On 2014/11/13 01:57:48, DaleCurtis wrote: > On 2014/11/13 01:54:39, prabhur1 wrote: > > Could you confirm if my assumption is correct about lifetime of > > MediaInternalsUMAHandler & the use of base::Unretained()? > > I didnt see any issues/crashes in local testing. > > Unretained is fine since this class is never destroyed :) Thanks! https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:253: PlayerInfoMap& player_info = renderer_info_[render_process_id]; On 2014/11/13 01:57:48, DaleCurtis wrote: > Add a DCHECK(CurrentlyOn::IO) ? Done. https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:296: if (player_info.has_video && player_info.has_audio) { On 2014/11/13 01:57:47, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/697463005/diff/120001/content/browser/media/m... content/browser/media/media_internals.cc:337: auto players_it = renderer_info_.find(render_process_id); On 2014/11/13 01:57:47, DaleCurtis wrote: > Ditto. Done.
The CQ bit was checked by prabhur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/697463005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/53bb9182b3e0ae046f228b921f03c13200aee7fe Cr-Commit-Position: refs/heads/master@{#303973} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
