Update use of MojoMediaLog without ThreadSafeAssociatedPtr

ThreadSafeAssociatedPtr is only supposed to be used by the IPC
conversion system.  In addition, it has a tendancy to post all
calls to MediaLog::AddEvent, which can have the side-effect of
ordering them after the (Mojo)VideoDecoder is destroyed since that
happens synchronously when VideoDecoder::Initialize fails.  So, we
don't get messages that describe why the failure happened, since
MojoVideoDecoderService has torn down the binding by then.

Instead, we make MojoMediaLog thread safe, and avoid the extra post
in the process.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I50fdb27c169c4b68222aa92f46c41a9ff23b4413
Reviewed-on: https://chromium-review.googlesource.com/1139080
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575722}

media/mojo/services/mojo_media_log.cc

@@ -5,12 +5,16 @@
 #include "media/mojo/services/mojo_media_log.h"
 
 #include "base/logging.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 
 namespace media {
 
-MojoMediaLog::MojoMediaLog(
-    scoped_refptr<mojom::ThreadSafeMediaLogAssociatedPtr> remote_media_log)
-    : remote_media_log_(std::move(remote_media_log)) {
+MojoMediaLog::MojoMediaLog(mojom::MediaLogAssociatedPtrInfo remote_media_log,
+                           scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : remote_media_log_(std::move(remote_media_log)),
+      task_runner_(std::move(task_runner)),
+      weak_ptr_factory_(this) {
+  weak_this_ = weak_ptr_factory_.GetWeakPtr();
   DVLOG(1) << __func__;
 }
 
@@ -21,7 +25,27 @@ MojoMediaLog::~MojoMediaLog() {
 void MojoMediaLog::AddEvent(std::unique_ptr<MediaLogEvent> event) {
   DVLOG(1) << __func__;
   DCHECK(event);
-  (**remote_media_log_).AddEvent(*event);
+
+  // Don't post unless we need to.  Otherwise, we can order a log entry after
+  // our own destruction.  While safe, this loses the message.  This can happen,
+  // for example, when we're logging why a VideoDecoder failed to initialize.
+  // It will be destroyed synchronously when Initialize returns.
+  //
+  // Also, we post here, so this is the base case.  :)
+  if (task_runner_->RunsTasksInCurrentSequence()) {
+    remote_media_log_->AddEvent(*event);
+    return;
+  }
+
+  // From other threads, it's okay to post without worrying about losing a
+  // message.  This is because any message that's causally related to the object
+  // (and thus MediaLog) being destroyed hopefully posts the result back to the
+  // same sequence as |task_runner_| after we do.  Of course, async destruction
+  // (e.g., the renderer destroys a MojoVideoDecoder) can still lose messages,
+  // but that's really a race.
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&MojoMediaLog::AddEvent, weak_this_, std::move(event)));
 }
 
 }  // namespace media

media/mojo/services/mojo_media_log.h

@@ -9,6 +9,8 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
 #include "media/base/media_log.h"
 #include "media/mojo/interfaces/media_log.mojom.h"
 
@@ -17,15 +19,23 @@ namespace media {
 class MojoMediaLog final : public MediaLog {
  public:
   // TODO(sandersd): Template on Ptr type to support non-associated.
-  explicit MojoMediaLog(
-      scoped_refptr<mojom::ThreadSafeMediaLogAssociatedPtr> remote_media_log);
+  explicit MojoMediaLog(mojom::MediaLogAssociatedPtrInfo remote_media_log,
+                        scoped_refptr<base::SequencedTaskRunner> task_runner);
   ~MojoMediaLog() final;
 
-  // MediaLog implementation.
+  // MediaLog implementation.  May be called from any thread, but will only
+  // use |remote_media_log_| on |task_runner_|.
   void AddEvent(std::unique_ptr<MediaLogEvent> event) override;
 
  private:
-  scoped_refptr<mojom::ThreadSafeMediaLogAssociatedPtr> remote_media_log_;
+  mojom::MediaLogAssociatedPtr remote_media_log_;
+
+  // The mojo service thread on which we'll access |remote_media_log_|.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  base::WeakPtr<MojoMediaLog> weak_this_;
+
+  base::WeakPtrFactory<MojoMediaLog> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MojoMediaLog);
 };

media/mojo/services/mojo_video_decoder_service.cc

@@ -136,9 +136,11 @@ void MojoVideoDecoderService::Construct(
 
   client_.Bind(std::move(client));
 
-  media_log_ = std::make_unique<MojoMediaLog>(
-      mojom::ThreadSafeMediaLogAssociatedPtr::Create(
-          std::move(media_log), base::ThreadTaskRunnerHandle::Get()));
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      base::ThreadTaskRunnerHandle::Get();
+
+  media_log_ =
+      std::make_unique<MojoMediaLog>(std::move(media_log), task_runner);
 
   video_frame_handle_releaser_ =
       mojo::MakeStrongBinding(std::make_unique<VideoFrameHandleReleaserImpl>(),
@@ -148,8 +150,7 @@ void MojoVideoDecoderService::Construct(
       new MojoDecoderBufferReader(std::move(decoder_buffer_pipe)));
 
   decoder_ = mojo_media_client_->CreateVideoDecoder(
-      base::ThreadTaskRunnerHandle::Get(), media_log_.get(),
-      std::move(command_buffer_id),
+      task_runner, media_log_.get(), std::move(command_buffer_id),
       base::Bind(&MojoVideoDecoderService::OnDecoderRequestedOverlayInfo,
                  weak_this_),
       target_color_space);