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

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

When a complete video frame is decoded, this CL converts the pixels from
BGRA to a format suitable for loading into a Java Bitmap directly. This
removes the need to create a temporary int[] array in Java.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226405

[Lambros, 2013-09-20 16:33:01]

This adds a VideoDecoderWrapper class as suggested by Wez to do the
byte-swapping. If you try out this CL, you will discover the bug that
https://codereview.chromium.org/24072012/ is intended to fix (partially
byte-swapped frames are seen during finger-swipes because repainting occurs at
the "wrong" time).

TODO: rebase this CL against the other one, and test whether that resolves the
problem.

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

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:21: #if defined(OS_ANDROID)
No need to wrap this include in ifdefs (even if you end up using it on android
only)

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:31: #if defined(OS_ANDROID)
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.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:32: class VideoDecoderWrapper :
public VideoDecoder {
Maybe call it RgbToBgrVideoDecoderFilter?

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:43: OVERRIDE {
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.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:47: virtual bool IsReadyForData()
OVERRIDE {
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

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:70: static const int
kBytesPerPixelRGB32 = 4;
Please add this const in VideoDecoder interface and then remove duplicate consts
from video_decoder_verbatim.cc and video_decoder_vp8.cc.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:75: libyuv::ABGRToARGB(pixels,
image_stride, pixels, image_stride,
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.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:118: #if defined (OS_ANDROID)
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.

https://codereview.chromium.org/23677011/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/23677011/diff/1/remoting/remoting.gyp#newcode...
remoting/remoting.gyp:2571: ['OS=="android"', {
You don't need this given that you also added an unconditional dependency above.

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

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:75: libyuv::ABGRToARGB(pixels,
image_stride, pixels, image_stride,
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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.

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]

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:75: libyuv::ABGRToARGB(pixels,
image_stride, pixels, image_stride,
On 2013/09/20 21:50:58, fbarchard wrote:
> On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> > 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.
> 
> 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.
> 

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

[fbarchard1, 2013-09-23 19:45:57]

On 2013/09/20 21:56:03, Sergey Ulanov wrote:
>
https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
> File remoting/client/rectangle_update_decoder.cc (right):
> 
>
https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
> remoting/client/rectangle_update_decoder.cc:75: libyuv::ABGRToARGB(pixels,
> image_stride, pixels, image_stride,
> On 2013/09/20 21:50:58, fbarchard wrote:
> > On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> > > 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.
> > 
> > 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.
> > 
> 
> Decoder uses media::ScaleYUVToRGB32WithRect() for scaling and YUV conversion,
> and it doesn't seem to support ABGR output.

libyuv::YUVToARGBScaleClip takes a destination fourcc for exactly this use case,
allowing any of the 4 ARGB swizzlings.

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

ptal

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:21: #if defined(OS_ANDROID)
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> No need to wrap this include in ifdefs (even if you end up using it on android
> only)

Done.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:31: #if defined(OS_ANDROID)
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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.

Done.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:32: class VideoDecoderWrapper :
public VideoDecoder {
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> Maybe call it RgbToBgrVideoDecoderFilter?

Done.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:43: OVERRIDE {
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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.

Hmm.. wrapping wasn't needed anyway.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:47: virtual bool IsReadyForData()
OVERRIDE {
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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

Done.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:70: static const int
kBytesPerPixelRGB32 = 4;
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> Please add this const in VideoDecoder interface and then remove duplicate
consts
> from video_decoder_verbatim.cc and video_decoder_vp8.cc.

Done.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:75: libyuv::ABGRToARGB(pixels,
image_stride, pixels, image_stride,
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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.

Added TODO.

https://codereview.chromium.org/23677011/diff/1/remoting/client/rectangle_upd...
remoting/client/rectangle_update_decoder.cc:118: #if defined (OS_ANDROID)
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> 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.

Done - added output format getter to FrameConsumer interface.

https://codereview.chromium.org/23677011/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/23677011/diff/1/remoting/remoting.gyp#newcode...
remoting/remoting.gyp:2571: ['OS=="android"', {
On 2013/09/20 18:35:15, Sergey Ulanov wrote:
> You don't need this given that you also added an unconditional dependency
above.

Removed.

[fbarchard1, 2013-09-26 00:31:54]

lgtm from libyuv::ABGRToARGB point of view.  I'll make a point to optimize this
function more - https://code.google.com/p/libyuv/issues/detail?id=271

[solb, 2013-09-26 14:34:08]

I don't know much about the VideoDecoder hierarchy, but from what I can see the
patch lgtm.  I couldn't test it, though, because it seems to still need some
rebasing?

[Lambros, 2013-09-26 20:06:28]

Rebased, should work now.

Sergey, I'll wait for your LG before landing this.

[Sergey Ulanov, 2013-09-27 21:51:13]

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
File remoting/client/frame_consumer.h (right):

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
remoting/client/frame_consumer.h:51: // from any thread.
Don't need the threading comment. This interface is not thread-safe. No need to
make it different for this method.

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
File remoting/client/frame_consumer_proxy.cc (right):

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
remoting/client/frame_consumer_proxy.cc:66: 
remove this line.

https://codereview.chromium.org/23677011/diff/30001/remoting/client/frame_con...
File remoting/client/frame_consumer_proxy.cc (right):

https://codereview.chromium.org/23677011/diff/30001/remoting/client/frame_con...
remoting/client/frame_consumer_proxy.cc:61: return pixel_format_;
This value is set on a different thread. It's not safe to read it here without a
lock. In the same time I don't think a lock is the right solution. I suggest the
following: 
 - Remove Attach() method and instead pass frame_consumer in the constructor.
Then it will be safe to read this value here without a lock.
 - In ChromotingInstance::ConnectWithConfig() you will need to create PepperView
(which is a FrameConsumer) before creating FrameConsumerProxy, but you need the
FrameConsumerProxy to create the client (which contains FrameProducer). This can
be solved by removing |producer| parameter from PepperView constructor. Instead
move it to the InitiateDrawing() method and call that method explicitly after
Client is created.

Currently objects are created in the following order: ConsumerProxy,
ChromotingClient (with RectangleUpdateDecoder), PepperView.
My suggestion above is essentially to change that order to the following:
PepperView, ConsumerProxy, ChromotingClient (with RectangleUpdateDecoder).

[Lambros, 2013-09-28 00:22:18]

ptal - I'm not sure PepperView::InitiateDrawing() is the best place to pass in
the FrameProducer*. Maybe we should just add a setter for this?

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
File remoting/client/frame_consumer.h (right):

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
remoting/client/frame_consumer.h:51: // from any thread.
On 2013/09/27 21:51:13, Sergey Ulanov wrote:
> Don't need the threading comment. This interface is not thread-safe. No need
to
> make it different for this method.

Done.

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
File remoting/client/frame_consumer_proxy.cc (right):

https://codereview.chromium.org/23677011/diff/16001/remoting/client/frame_con...
remoting/client/frame_consumer_proxy.cc:66: 
On 2013/09/27 21:51:13, Sergey Ulanov wrote:
> remove this line.

Done.

https://codereview.chromium.org/23677011/diff/30001/remoting/client/frame_con...
File remoting/client/frame_consumer_proxy.cc (right):

https://codereview.chromium.org/23677011/diff/30001/remoting/client/frame_con...
remoting/client/frame_consumer_proxy.cc:61: return pixel_format_;
On 2013/09/27 21:51:13, Sergey Ulanov wrote:
> This value is set on a different thread. It's not safe to read it here without
a
> lock. In the same time I don't think a lock is the right solution. I suggest
the
> following: 
>  - Remove Attach() method and instead pass frame_consumer in the constructor.
> Then it will be safe to read this value here without a lock.
>  - In ChromotingInstance::ConnectWithConfig() you will need to create
PepperView
> (which is a FrameConsumer) before creating FrameConsumerProxy, but you need
the
> FrameConsumerProxy to create the client (which contains FrameProducer). This
can
> be solved by removing |producer| parameter from PepperView constructor.
Instead
> move it to the InitiateDrawing() method and call that method explicitly after
> Client is created.
> 
> Currently objects are created in the following order: ConsumerProxy,
> ChromotingClient (with RectangleUpdateDecoder), PepperView.
> My suggestion above is essentially to change that order to the following:
> PepperView, ConsumerProxy, ChromotingClient (with RectangleUpdateDecoder).

Done.

[Sergey Ulanov, 2013-09-28 00:33:31]

https://codereview.chromium.org/23677011/diff/36001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_view.h (right):

https://codereview.chromium.org/23677011/diff/36001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.h:70: void InitiateDrawing(FrameProducer*
producer);
nit: Please rename it to Start() or Initialize(). Also put it right after the
destructor, so it's clear that it should be called first.

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:56: // class.
nit: Don't need this comment here. it might be better to a comment for the whole
class.

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:57: // TODO(lambroslambrou):
Refactor so that the VideoDecoder produces data
Move this TODO above the class definition - we won't need this class when the
TODO is addressed.

[Sergey Ulanov, 2013-09-28 00:33:44]

lgtm when my nits are addressed

[Lambros, 2013-09-28 00:51:10]

https://codereview.chromium.org/23677011/diff/36001/remoting/client/plugin/pe...
File remoting/client/plugin/pepper_view.h (right):

https://codereview.chromium.org/23677011/diff/36001/remoting/client/plugin/pe...
remoting/client/plugin/pepper_view.h:70: void InitiateDrawing(FrameProducer*
producer);
On 2013/09/28 00:33:32, Sergey Ulanov wrote:
> nit: Please rename it to Start() or Initialize(). Also put it right after the
> destructor, so it's clear that it should be called first.

Done.

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
File remoting/client/rectangle_update_decoder.cc (right):

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:56: // class.
On 2013/09/28 00:33:32, Sergey Ulanov wrote:
> nit: Don't need this comment here. it might be better to a comment for the
whole
> class.

Done.

https://codereview.chromium.org/23677011/diff/36001/remoting/client/rectangle...
remoting/client/rectangle_update_decoder.cc:57: // TODO(lambroslambrou):
Refactor so that the VideoDecoder produces data
On 2013/09/28 00:33:32, Sergey Ulanov wrote:
> Move this TODO above the class definition - we won't need this class when the
> TODO is addressed.

Done.

[commit-bot: I haz the power, 2013-09-28 00:54:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/23677011/4...

[commit-bot: I haz the power, 2013-09-28 04:01:07]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[solb, 2013-09-28 05:04:08]

My tests of this patch seem to indicate that it breaks rendering:  I first
tested connecting from a client built at Git c42fc13 to Ubuntu Precise x64 and
Win7 Pro x64 (no curtain mode) VMs; this worked without issue.  However, after
rebuilding with the patch applied, I see only the first frame of the connection
and get no further updates, despite being able to send mouse and keyboard input
events.

[Lambros, 2013-09-30 21:45:55]

Yeah, it was working fine on my Nexus 7 but not on my Nexus 4. Adding rewind()
fixes this (see latest patch), though my other CL
(https://codereview.chromium.org/24072012/) should make the rewind()
unnecessary.

[fbarchard1, 2013-10-01 07:04:43]

FYI I've ported the AVX2 version of ABGRToARGB to OSX
https://webrtc-codereview.appspot.com/2320005/



On Mon, Sep 30, 2013 at 2:45 PM, <lambroslambrou@chromium.org> wrote:

> Yeah, it was working fine on my Nexus 7 but not on my Nexus 4. Adding
> rewind()
> fixes this (see latest patch), though my other CL
>
(https://codereview.chromium.**org/24072012/<https://codereview.chromium.org/2...)
> should make the rewind()
> unnecessary.
>
>
https://codereview.chromium.**org/23677011/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[solb, 2013-10-01 14:00:20]

On 2013/09/30 21:45:55, Lambros wrote:
> Yeah, it was working fine on my Nexus 7 but not on my Nexus 4. Adding rewind()
> fixes this (see latest patch), though my other CL
> (https://codereview.chromium.org/24072012/) should make the rewind()
> unnecessary.

That line fixes the updating issue for me.  Everything looks good as far as I
can see!

[commit-bot: I haz the power, 2013-10-01 22:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/23677011/7...

[commit-bot: I haz the power, 2013-10-02 00:00:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 01:42:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/23677011/1...

[commit-bot: I haz the power, 2013-10-02 06:53:59]

Change committed as 226405