Replace Acquire_Store calls with Release_Store as these stores should have the release ordering/sem…

chromecast/media/cma/ipc/media_message_fifo.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 "chromecast/media/cma/ipc/media_message_fifo.h"
 
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 105 matching lines @@
   rd_offset_ = reinterpret_cast<AtomicSize*>(&(desc->rd_offset));
   wr_offset_ = reinterpret_cast<AtomicSize*>(&(desc->wr_offset));
 
   size_t max_size = mem_->size() -
       (static_cast<char*>(base_) - static_cast<char*>(mem_->data()));
   if (init) {
     size_ = max_size;
     desc->size = size_;
     internal_rd_offset_ = 0;
     internal_wr_offset_ = 0;
-    base::subtle::Acquire_Store(rd_offset_, 0);
-    base::subtle::Acquire_Store(wr_offset_, 0);
+    base::subtle::Release_Store(rd_offset_, 0);

[servolk, 2014/10/07 19:43:37]

The bug referenced in the description
(https://code.google.com/p/chromium/issues/detail?id=420970) talks about
removing BOTH Acquire_Store and Release_Store and replacing them with proper
C++11 atomics. So why not use C++11 atomics here (std::atomic_store, I suppose)?
Or is that something that Chromecast can't support yet due to being on older
GCC?

[lcwu1, 2014/10/07 21:30:17]

No, the bug was talking about removing Acquire_Store and Release_Load (not store
here) because the new C++ memory model doesn't define their behavior. (C++
memory model only defines the behavior of Release_Store and Acquire_Load, both
of which are most commonly used.)

Basically the upstream is working on replacing the implementation of
base/atomicops operations with C++11's new std::atomics so that the existing
calls to base/atomicops don't have to change. But our code would stop to work if
we don't upgrade to a new toolchain that supports C++11 atomics.

+    base::subtle::Release_Store(wr_offset_, 0);
   } else {
     size_ = desc->size;
     CHECK_LE(size_, max_size);
     internal_rd_offset_ = current_rd_offset();
     internal_wr_offset_ = current_wr_offset();
   }
   CMALOG(kLogControl)
       << "MediaMessageFifo:" << " init=" << init << " size=" << size_;
   CHECK_GT(size_, 0) << size_;
 
@@ skipping 225 matching lines @@
   size_t wr_offset = base::subtle::Acquire_Load(wr_offset_);
   CHECK_LT(wr_offset, size_);
   return wr_offset;
 }
 
 void MediaMessageFifo::CommitRead(size_t new_rd_offset) {
   // Add a memory fence to ensure the message content is completely read
   // before updating the read offset.
   base::subtle::Release_Store(rd_offset_, new_rd_offset);
 
   // Make sure the read pointer has been updated before sending a notification.

[damienv1, 2014/10/07 19:22:31]

The comment should be updated (just to mention it's safe to send the
notification: the other thread will get the latest value for the read offset
through an Acquire_Load).

[lcwu1, 2014/10/07 21:30:17]

Done.

   if (!read_event_cb_.is_null()) {
-    base::subtle::MemoryBarrier();
     read_event_cb_.Run();
   }
 }
 
 void MediaMessageFifo::CommitWrite(size_t new_wr_offset) {
   // Add a memory fence to ensure the message content is written
   // before updating the write offset.
   base::subtle::Release_Store(wr_offset_, new_wr_offset);
 
   // Make sure the write pointer has been updated before sending a notification.

[damienv1, 2014/10/07 19:22:30]

Same: update the comment.
Also, could you please make sure that running TSAN on the unit test is still
fine.

[lcwu1, 2014/10/07 21:30:17]

Ran Tsan on cast_media_unittests and everything is fine. (To make tsan run, I
had to fix a couple of issues in ozone, which I will prepare another CL for.)

   if (!write_event_cb_.is_null()) {
-    base::subtle::MemoryBarrier();
     write_event_cb_.Run();
   }
 }
 
 void MediaMessageFifo::CommitInternalRead(size_t new_rd_offset) {
   internal_rd_offset_ = new_rd_offset;
 }
 
 void MediaMessageFifo::CommitInternalWrite(size_t new_wr_offset) {
   internal_wr_offset_ = new_wr_offset;
 }
 
 }  // namespace media
 }  // namespace chromecast