Byte-swap the video frame pixels before passing them to Java.

remoting/client/rectangle_update_decoder.cc

 // Copyright (c) 2012 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 "remoting/client/rectangle_update_decoder.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "remoting/base/util.h"
 #include "remoting/codec/video_decoder.h"
 #include "remoting/codec/video_decoder_verbatim.h"
 #include "remoting/codec/video_decoder_vp8.h"
 #include "remoting/client/frame_consumer.h"
 #include "remoting/protocol/session_config.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
+#if defined(OS_ANDROID)

[Sergey Ulanov, 2013/09/20 18:35:15]

No need to wrap this include in ifdefs (even if you end up using it on android
only)

[Lambros, 2013/09/25 18:40:41]

Done.

+#include "third_party/libyuv/include/libyuv/convert_argb.h"
+#endif  // OS_ANDROID
+
 using base::Passed;
 using remoting::protocol::ChannelConfig;
 using remoting::protocol::SessionConfig;
 
 namespace remoting {
 
+#if defined(OS_ANDROID)

[Sergey Ulanov, 2013/09/20 18:35:15]

Can we make this more generic so it's easier to reuse in case we need ABGR on
other platform. There is nothing platform specific in this class, so it doesn't
need to be inside an ifded.

[Lambros, 2013/09/25 18:40:41]

Done.

+class VideoDecoderWrapper : public VideoDecoder {

[Sergey Ulanov, 2013/09/20 18:35:15]

Maybe call it RgbToBgrVideoDecoderFilter?

[Lambros, 2013/09/25 18:40:41]

Done.

+ public:
+  VideoDecoderWrapper(scoped_ptr<VideoDecoder> parent)
+      : parent_(parent.Pass()) {
+  }
+
+  virtual void Initialize(const SkISize& screen_size) OVERRIDE {
+    parent_->Initialize(screen_size);
+  }
+
+  virtual DecodeResult DecodePacket(const VideoPacket* packet)
+      OVERRIDE {

[Sergey Ulanov, 2013/09/20 18:35:15]

nit: OVERRIDE must be on one line with the last parameter, i.e. you need to wrap
the parameter here, if OVERRIDE doesn't fit on the previous line.

[Lambros, 2013/09/25 18:40:41]

Hmm.. wrapping wasn't needed anyway.

+    return parent_->DecodePacket(packet);
+  }
+
+  virtual bool IsReadyForData() OVERRIDE {

[Sergey Ulanov, 2013/09/20 18:35:15]

VideoDecoder interface has rotten over time. There are some easy cleanup that
can be done in that interface.
 - IsReadyForData() always returns true in all implementation. Don't need it.
 - Encoding() is never called. Don't need it.
 - DecodePacket() can return bool instead of DecodeResult.

It would be great if you could do these cleanups before adding a new
VideoDecoder

[Lambros, 2013/09/25 18:40:41]

Done.

+    return parent_->IsReadyForData();
+  }
+
+  virtual VideoPacketFormat::Encoding Encoding() OVERRIDE {
+    return parent_->Encoding();
+  }
+
+  virtual void Invalidate(const SkISize& view_size,
+                          const SkRegion& region) OVERRIDE {
+    return parent_->Invalidate(view_size, region);
+  }
+
+  virtual void RenderFrame(const SkISize& view_size,
+                           const SkIRect& clip_area,
+                           uint8* image_buffer,
+                           int image_stride,
+                           SkRegion* output_region) OVERRIDE {
+    parent_->RenderFrame(view_size, clip_area, image_buffer, image_stride,
+                         output_region);
+
+    // Byte-swap the pixels for compatibility with the android.graphics.Bitmap
+    // class.
+    static const int kBytesPerPixelRGB32 = 4;

[Sergey Ulanov, 2013/09/20 18:35:15]

Please add this const in VideoDecoder interface and then remove duplicate consts
from video_decoder_verbatim.cc and video_decoder_vp8.cc.

[Lambros, 2013/09/25 18:40:41]

Done.

+    for (SkRegion::Iterator i(*output_region); !i.done(); i.next()) {
+      SkIRect rect = i.rect();
+      uint8* pixels = image_buffer + (rect.top() * image_stride) +
+        (rect.left() * kBytesPerPixelRGB32);
+      libyuv::ABGRToARGB(pixels, image_stride, pixels, image_stride,

[Sergey Ulanov, 2013/09/20 18:35:15]

It would be great if we could get data in the right byteorder from the decoder,
instead of swapping it here. That's especially important on low-power devices.
Might be easy to fix when we switch to VP9. Please add TODO to fix it.

[fbarchard, 2013/09/20 21:50:58]

If this is coming from the codec as I420, libyuv can convert directly to ABGR.
I420ToABGR(). But on capture side, its likely ARGB and can't be captured as
ABGR.

[Sergey Ulanov, 2013/09/20 21:56:03]

Decoder uses media::ScaleYUVToRGB32WithRect() for scaling and YUV conversion,
and it doesn't seem to support ABGR output.

[Lambros, 2013/09/25 18:40:41]

Added TODO.

+                         rect.width(), rect.height());
+    }
+  }
+
+  virtual const SkRegion* GetImageShape() OVERRIDE {
+    return parent_->GetImageShape();
+  }
+
+ private:
+  scoped_ptr<VideoDecoder> parent_;
+};
+#endif  // OS_ANDROID
+
 RectangleUpdateDecoder::RectangleUpdateDecoder(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> decode_task_runner,
     scoped_refptr<FrameConsumerProxy> consumer)
     : main_task_runner_(main_task_runner),
       decode_task_runner_(decode_task_runner),
       consumer_(consumer),
       source_size_(SkISize::Make(0, 0)),
       source_dpi_(SkIPoint::Make(0, 0)),
       view_size_(SkISize::Make(0, 0)),
       clip_area_(SkIRect::MakeEmpty()),
       paint_scheduled_(false),
       latest_sequence_number_(0) {
 }
 
 RectangleUpdateDecoder::~RectangleUpdateDecoder() {
 }
 
 void RectangleUpdateDecoder::Initialize(const SessionConfig& config) {
   // Initialize decoder based on the selected codec.
   ChannelConfig::Codec codec = config.video_config().codec;
   if (codec == ChannelConfig::CODEC_VERBATIM) {
     decoder_.reset(new VideoDecoderVerbatim());
   } else if (codec == ChannelConfig::CODEC_VP8) {
     decoder_.reset(new VideoDecoderVp8());
   } else {
     NOTREACHED() << "Invalid Encoding found: " << codec;
   }
+
+#if defined (OS_ANDROID)

[Sergey Ulanov, 2013/09/20 18:35:15]

I think it's better to make this more generic. There is no reason you need to
add ifdefs. I suggest adding a parameter in Initialize() to indicated what
output format is expected.

[Lambros, 2013/09/25 18:40:41]

Done - added output format getter to FrameConsumer interface.

+  scoped_ptr<VideoDecoder> wrapper(new VideoDecoderWrapper(decoder_.Pass()));
+  decoder_ = wrapper.Pass();
+#endif  // OS_ANDROID
 }
 
 void RectangleUpdateDecoder::DecodePacket(scoped_ptr<VideoPacket> packet,
                                           const base::Closure& done) {
   DCHECK(decode_task_runner_->BelongsToCurrentThread());
 
   base::ScopedClosureRunner done_runner(done);
 
   bool decoder_needs_reset = false;
   bool notify_size_or_dpi_change = false;
@@ skipping 215 matching lines @@
 
   done.Run();
 }
 
 ChromotingStats* RectangleUpdateDecoder::GetStats() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   return &stats_;
 }
 
 }  // namespace remoting