Add tracing AutoOpenCloseEvent.

media/blink/video_frame_compositor.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/blink/video_frame_compositor.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
+#include "base/trace_event/video_playback_pae.h"
 #include "media/base/video_frame.h"
 
 namespace media {
 
 // Amount of time to wait between UpdateCurrentFrame() callbacks before starting
 // background rendering to keep the Render() callbacks moving.
 const int kBackgroundRenderingTimeoutMs = 250;
 
 VideoFrameCompositor::VideoFrameCompositor(
     const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner)
     : compositor_task_runner_(compositor_task_runner),
       tick_clock_(new base::DefaultTickClock()),
       background_rendering_enabled_(true),
       background_rendering_timer_(
           FROM_HERE,
           base::TimeDelta::FromMilliseconds(kBackgroundRenderingTimeoutMs),
           base::Bind(&VideoFrameCompositor::BackgroundRender,
                      base::Unretained(this)),
           // Task is not repeating, CallRender() will reset the task as needed.
           false),
       client_(nullptr),
       rendering_(false),
       rendered_last_frame_(false),
       is_background_rendering_(false),
       new_background_frame_(false),
       // Assume 60Hz before the first UpdateCurrentFrame() call.
       last_interval_(base::TimeDelta::FromSecondsD(1.0 / 60)),
-      callback_(nullptr) {
+      callback_(nullptr),
+      persistent_async_(
+          new base::trace_event::VideoPlaybackPersistentAsyncEvent()) {
   background_rendering_timer_.SetTaskRunner(compositor_task_runner_);
 }
 
 VideoFrameCompositor::~VideoFrameCompositor() {
   DCHECK(compositor_task_runner_->BelongsToCurrentThread());
   DCHECK(!callback_);
   DCHECK(!rendering_);
   if (client_)
     client_->StopUsingProvider();
 }
@@ skipping 49 matching lines @@
   DCHECK(compositor_task_runner_->BelongsToCurrentThread());
   return CallRender(deadline_min, deadline_max, false);
 }
 
 bool VideoFrameCompositor::HasCurrentFrame() {
   DCHECK(compositor_task_runner_->BelongsToCurrentThread());
   return static_cast<bool>(current_frame_);
 }
 
 void VideoFrameCompositor::Start(RenderCallback* callback) {
-  TRACE_EVENT_ASYNC_BEGIN0("media,rail", "VideoPlayback",

[DaleCurtis, 2016/08/30 19:09:49]

Do we really need a Start()/Stop() API or should this just construct an ASYNC
event during VFC::Start() and destruct it during VFC::Stop()? The recording can
be handled by the constructor and destructor respectively.

[alexandermont, 2016/08/30 19:34:36]

I don't really see an advantage of doing it the way you're suggesting. It's
possible to start and stop an event multiple times - and sometimes this will
happen, for instance if you're starting and stopping the same video multiple
times. Also, destructors can get called when you didn't do anything that makes
it obvious that the destructor was called (like calling delete) - if an object
goes out of scope that could trigger a destructor call. (Similar things can
happen with constructors as well - e.g. if the PersistentAsyncEvent is a member
of the class, then the constructor will be called when the class constructor is
called.) Having the recording happening in the constructor and destructor seems
like it might make it less clear to the reader of the code exactly when
recording starts and stops.

[DaleCurtis, 2016/08/30 19:42:31]

Generally I find it preferable to avoid unnecessary APIs for simplicity. We're
all well versed with smart pointers, and even have things like ElapsedTimer,
ScopedHistogramTimer, etc which perform in a similar vein. It's not media/ code
so I don't feel strongly, but I encourage you to avoid unnecessary function
calls for long term cleanliness. See a recent example of a similar cleanup I
did: https://codereview.chromium.org/2076793002/

[alexandermont, 2016/08/31 18:31:59]

See discussion below in comments. Given that future subclasses may add arguments
to the Start() and Stop() calls, we think it's best to leave them as specific
calls.

-                           static_cast<const void*>(this));
+  persistent_async_->Start();
 
   // Called from the media thread, so acquire the callback under lock before
   // returning in case a Stop() call comes in before the PostTask is processed.
   base::AutoLock lock(callback_lock_);
   DCHECK(!callback_);
   callback_ = callback;
   compositor_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VideoFrameCompositor::OnRendererStateUpdate,
                             base::Unretained(this), true));
 }
 
 void VideoFrameCompositor::Stop() {
-  TRACE_EVENT_ASYNC_END0("media,rail", "VideoPlayback",
-                         static_cast<const void*>(this));
+  persistent_async_->Stop();
 
   // Called from the media thread, so release the callback under lock before
   // returning to avoid a pending UpdateCurrentFrame() call occurring before
   // the PostTask is processed.
   base::AutoLock lock(callback_lock_);
   DCHECK(callback_);
   callback_ = nullptr;
   compositor_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VideoFrameCompositor::OnRendererStateUpdate,
                             base::Unretained(this), false));
@@ skipping 107 matching lines @@
   last_interval_ = deadline_max - deadline_min;
 
   // Restart the background rendering timer whether we're background rendering
   // or not; in either case we should wait for |kBackgroundRenderingTimeoutMs|.
   if (background_rendering_enabled_)
     background_rendering_timer_.Reset();
   return new_frame || had_new_background_frame;
 }
 
 }  // namespace media