ppapi: Change endianness in libyuv calls, from BGRA to ARGB

content/renderer/media/webrtc/video_destination_handler.cc

Index: content/renderer/media/webrtc/video_destination_handler.cc
diff --git a/content/renderer/media/webrtc/video_destination_handler.cc b/content/renderer/media/webrtc/video_destination_handler.cc
index 160537165d7912e4c514f6d49272f9dd60d0adde..a73ee21d70ddcefe5071960536dcf5c8a1a3e7c0 100644
--- a/content/renderer/media/webrtc/video_destination_handler.cc
+++ b/content/renderer/media/webrtc/video_destination_handler.cc
@@ -41,6 +41,13 @@ class PpFrameWriter::FrameWriterDelegate
 
  private:
   friend class base::RefCountedThreadSafe<FrameWriterDelegate>;
+  // Endian in memory order, e.g. AXXX stands for uint8 pixel[4] = {A, x, x, x};
+  enum PixelEndian {
+    UNKNOWN,
+    AXXX,
+    XXXA,
+  };
+
   virtual ~FrameWriterDelegate();
 
   void StartDeliverOnIO(const VideoCaptureDeliverFrameCB& frame_callback);
@@ -52,9 +59,11 @@ class PpFrameWriter::FrameWriterDelegate
 
   scoped_refptr<base::MessageLoopProxy> io_message_loop_;
 
-  // |frame_pool_| and |new_frame_callback_| are only used on the IO-thread.
+  // |frame_pool_|, |new_frame_callback_| and |endian_| are only used on the
+  // IO-thread.
   media::VideoFramePool frame_pool_;
   VideoCaptureDeliverFrameCB new_frame_callback_;
+  PixelEndian endian_;
 
   // Used to DCHECK that we are called on the main render thread.
   base::ThreadChecker thread_checker_;
@@ -62,7 +71,7 @@ class PpFrameWriter::FrameWriterDelegate
 
 PpFrameWriter::FrameWriterDelegate::FrameWriterDelegate(
     const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy)
-    : io_message_loop_(io_message_loop_proxy) {
+    : io_message_loop_(io_message_loop_proxy), endian_(UNKNOWN) {
 }
 
 PpFrameWriter::FrameWriterDelegate::~FrameWriterDelegate() {
@@ -101,6 +110,8 @@ void PpFrameWriter::FrameWriterDelegate::DeliverFrame(
                << "The image_data's mapped bitmap is NULL.";
     return;
   }
+  // We only support PP_IMAGEDATAFORMAT_BGRA_PREMUL at the moment.
+  DCHECK(image_data->format() == PP_IMAGEDATAFORMAT_BGRA_PREMUL);
   io_message_loop_->PostTaskAndReply(
       FROM_HERE,
       base::Bind(&FrameWriterDelegate::DeliverFrameOnIO, this,
@@ -153,15 +164,43 @@ void PpFrameWriter::FrameWriterDelegate::DeliverFrameOnIO(
       MediaStreamVideoSource::kUnknownFrameRate,
       media::PIXEL_FORMAT_YV12);
 
-  libyuv::BGRAToI420(data,
-                     stride,
-                     new_frame->data(media::VideoFrame::kYPlane),
-                     new_frame->stride(media::VideoFrame::kYPlane),
-                     new_frame->data(media::VideoFrame::kUPlane),
-                     new_frame->stride(media::VideoFrame::kUPlane),
-                     new_frame->data(media::VideoFrame::kVPlane),
-                     new_frame->stride(media::VideoFrame::kVPlane),
-                     frame_size.width(), frame_size.height());
+  // TODO(magjed): Remove this and always use libyuv::ARGBToI420 when
+  // crbug/426020 is fixed.
+  // Due to confusion about endianness, we try to determine it from the data.

[fbarchard, 2014/10/22 21:47:43]

I'm not aware of any 'confusion'.  Pepper originally only supported BGRA - big
endian, A,R,G,B.   Now that we can use ARGB, we're switching.  But we dont have
an easy way to confirm the chrome version, so we're auto detecting the pixel
channel order.

[magjed_chromium, 2014/10/23 10:59:35]

I removed 'confusion' from the comment. Something is inconsistent in the current
code though, because the comments for PP_ImageDataFormat state:
* The third part of each enumeration value describes the memory layout from
 * the lowest address to the highest. For example, BGRA means the B component
 * is stored in the lowest address, no matter what endianness the platform is
 * using.
and this is currently not true. Link to PP_ImageDataFormat:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_image_...

+  // The alpha channel is always 255. It is unlikely for other color channels to
+  // be 255, so we will most likely break on the first few pixels in the first
+  // frame.
+  uint8* row_ptr = data;
+  for (int y = 0; y < height && endian_ == UNKNOWN; ++y) {

[fbarchard, 2014/10/22 21:47:43]

consider making endian_ static so it detects once.

[magjed_chromium, 2014/10/23 10:59:35]

I prefer to keep it per stream. static variables for POD is allowed, but not
encouraged. We recently moved the color conversion to the IO thread in this CL:
https://codereview.chromium.org/631903003/
so it should be thread safe now, but we might revert that CL in the future.
Also, making it static is a minor optimization.

+    for (int x = 0; x < width; ++x) {
+      if (row_ptr[x * 4 + 0] != 255) {  // First byte is not Alpha => XXXA.
+        endian_ = XXXA;
+        break;
+      }
+      if (row_ptr[x * 4 + 3] != 255) {  // Fourth byte is not Alpha => AXXX.
+        endian_ = AXXX;
+        break;
+      }
+    }
+    row_ptr += stride;
+  }
+  if (endian_ == UNKNOWN) {
+    LOG(WARNING) << "PpFrameWriter::FrameWriterDelegate::DeliverFrameOnIO - "
+                 << "Could not determine endianness.";
+  }
+  // libyuv uses reversed memory byte order, that is why the naming is reversed.

[fbarchard, 2014/10/22 21:47:43]

comment misleading.  libyuv specifies fourcc/channel ordering the same as
webrtc.
FYI Chrome supports 4 channel orderings: ARGB, BGRA, RGBA, and ABGR.

[magjed_chromium, 2014/10/23 10:59:36]

Done.

+  const auto xxxxToI420 =
+      (endian_ == AXXX) ? libyuv::BGRAToI420 : libyuv::ARGBToI420;
+  xxxxToI420(data,

[fbarchard, 2014/10/22 21:47:43]

consider using ConvertToI420, which takes the fourcc as a parameter.

is 'auto' safe to use?

[magjed_chromium, 2014/10/23 10:59:36]

I can't pass the src stride to ConvertToI420 - it assumes it is tightly packed.
'auto' was recently approved for chromium, ref: http://chromium-cpp.appspot.com/

+             stride,
+             new_frame->data(media::VideoFrame::kYPlane),
+             new_frame->stride(media::VideoFrame::kYPlane),
+             new_frame->data(media::VideoFrame::kUPlane),
+             new_frame->stride(media::VideoFrame::kUPlane),
+             new_frame->data(media::VideoFrame::kVPlane),
+             new_frame->stride(media::VideoFrame::kVPlane),
+             width,
+             height);
 
   // The local time when this frame is generated is unknown so give a null
   // value to |estimated_capture_time|.