Apply rotation for texture backed VideoFrames

content/renderer/media/webmediaplayer_ms.cc

Index: content/renderer/media/webmediaplayer_ms.cc
diff --git a/content/renderer/media/webmediaplayer_ms.cc b/content/renderer/media/webmediaplayer_ms.cc
index b03fedd8d5365e00e9c19a7bc58af7418c6bf407..978591f8387d41ba999e99a137cc83840b5a9f38 100644
--- a/content/renderer/media/webmediaplayer_ms.cc
+++ b/content/renderer/media/webmediaplayer_ms.cc
@@ -25,6 +25,7 @@
 #include "content/renderer/render_thread_impl.h"
 #include "media/base/media_log.h"
 #include "media/base/video_frame.h"
+#include "media/base/video_rotation.h"
 #include "media/blink/webmediaplayer_util.h"
 #include "third_party/WebKit/public/platform/WebMediaPlayerClient.h"
 #include "third_party/WebKit/public/platform/WebMediaPlayerSource.h"
@@ -55,6 +56,7 @@ WebMediaPlayerMS::WebMediaPlayerMS(
       paused_(true),
       render_frame_suspended_(false),
       received_first_frame_(false),
+      video_rotation_(media::VIDEO_ROTATION_0),
       media_log_(media_log),
       renderer_factory_(std::move(factory)),
       media_task_runner_(media_task_runner),
@@ -264,6 +266,11 @@ bool WebMediaPlayerMS::hasAudio() const {
 
 blink::WebSize WebMediaPlayerMS::naturalSize() const {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (video_rotation_ == media::VIDEO_ROTATION_90 ||
+      video_rotation_ == media::VideoRotation::VIDEO_ROTATION_270) {
+    const gfx::Size& current_size = compositor_->GetCurrentSize();
+    return blink::WebSize(current_size.height(), current_size.width());

[mcasas, 2016/07/15 01:57:34]

nit: blink::WebSize has a ctor with gfx::Size as parameter [1]
so you can do
return blink::WebSize(compositor->GetCurrentSize());



[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSi...

[chcunningham, 2016/07/15 18:25:52]

I think he means to swap width and height here. IIUC your suggestion wouldn't do
the rotation.

[emircan, 2016/07/15 18:43:13]

I am swapping height and width here as ccunningham@ pointed out.

+  }
   return blink::WebSize(compositor_->GetCurrentSize());
 }
 
@@ -345,7 +352,7 @@ void WebMediaPlayerMS::paint(blink::WebCanvas* canvas,
   }
   const gfx::RectF dest_rect(rect.x, rect.y, rect.width, rect.height);
   video_renderer_.Paint(frame, canvas, dest_rect, alpha, mode,
-                        media::VIDEO_ROTATION_0, context_3d);
+                        video_rotation_, context_3d);
 }
 
 bool WebMediaPlayerMS::hasSingleSecurityOrigin() const {
@@ -494,8 +501,13 @@ void WebMediaPlayerMS::OnFrameAvailable(
     }
 
     if (video_frame_provider_.get()) {
+      if (frame->metadata()->GetRotation(media::VideoFrameMetadata::ROTATION,
+                                         &video_rotation_)) {
+        DVLOG(3) << "Applying rotation: " << video_rotation_;
+      }

[mcasas, 2016/07/15 01:57:34]

All metadata() methods are marked WARN_UNUSED_RESULT,
but you don't have to use a DVLOG just for that. Instead, 
please use ignore_result() to signify that here, you don't care.

https://cs.chromium.org/chromium/src/base/macros.h?sq=package:chromium&dr=C&r...

[chcunningham, 2016/07/15 18:25:52]

He does care though right? Line 510 where he uses video_rotation_

[emircan, 2016/07/15 18:43:13]

What I ignore here is the return bool of GetRotation, video_rotation_ is used in
all cases.

[emircan, 2016/07/15 18:43:13]

Done.

+
       video_weblayer_.reset(new cc_blink::WebLayerImpl(
-          cc::VideoLayer::Create(compositor_.get(), media::VIDEO_ROTATION_0)));
+          cc::VideoLayer::Create(compositor_.get(), video_rotation_)));
       video_weblayer_->layer()->SetContentsOpaque(false);
       video_weblayer_->SetContentsOpaqueIsFixed(true);
       get_client()->setWebLayer(video_weblayer_.get());