VideoTrackRecorder: add support for texture backed ARGB VideoFrames

content/renderer/media/video_track_recorder.cc

Index: content/renderer/media/video_track_recorder.cc
diff --git a/content/renderer/media/video_track_recorder.cc b/content/renderer/media/video_track_recorder.cc
index d65e238e8795e5d07b6ffcba163d6c135fb684c0..9fc9096d6579c6bfc8bd2e1a9098c545cd7761ac 100644
--- a/content/renderer/media/video_track_recorder.cc
+++ b/content/renderer/media/video_track_recorder.cc
@@ -185,14 +185,43 @@ void VideoTrackRecorder::Encoder::StartFrameEncode(
 
   if (!(video_frame->format() == media::PIXEL_FORMAT_I420 ||
         video_frame->format() == media::PIXEL_FORMAT_YV12 ||
+        video_frame->format() == media::PIXEL_FORMAT_ARGB ||
         video_frame->format() == media::PIXEL_FORMAT_YV12A)) {
-    NOTREACHED();
+    NOTREACHED() << media::VideoPixelFormatToString(video_frame->format());
     return;
   }

[emircan, 2016/07/12 22:57:00]

This code looks very similar to CopyFrame() below, which handles texture
download as well. Can we refactor these into a common place?

Also, I am not sure about sending black frames as a solution. AFAIK, HW decode
is common in Android which sends textures that aren't mappable. SW decode would
send mappable I420. So, ARGB&Mappable() case is probably not going to be called.


Seeing lots of black frames may confuse the developer as well. Is there any way
we can notify about this in console? Or just download the texture like
CopyFrame() below.

https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms...

[mcasas, 2016/07/13 00:45:30]

Happy to do that, but I see that CopyFrame() has a parameter 
media::SkCanvasVideoRenderer* video_renderer
that holds the data we want to extract; in this case we have 
no such luxury and we simply know that we can't map the 
frame. WDYT?
Hmmm then I guess it doesn't make sense to have
such case  :)  I'll remove it if this is the case, but
since there's a bit of discussion now, I'll hold the 
removal until your next round of comments.
I thought that is better for the user to record black
frames, meaning "data couldn't be accessed" rather
than not recording anything, which developers (like
cpaulin@) interpreted as "the code is broken". 

Plus, if down the line we find a method to encode 
these non-mappable frames (e.g. using Android
MediaCodec) we can start encoding properly.

-  scoped_refptr<media::VideoFrame> frame = video_frame;
-  // Drop alpha channel since we do not support it yet.
-  if (frame->format() == media::PIXEL_FORMAT_YV12A)
+  scoped_refptr<media::VideoFrame> frame;
+  if (video_frame->format() == media::PIXEL_FORMAT_ARGB) {
+    // Certain Android (hardware) decoders produce media::PIXEL_FORMAT_ARGB,
+    // (see https://crbug.com/585242) that may or may not be opaque. In the
+    // former case, record black frames (yuv = {0, 0x80, 0x80}) instead.
+    if (!video_frame->IsMappable()) {
+      frame = media::VideoFrame::CreateColorFrame(
+          video_frame->coded_size(), 0u, 0x80, 0x80, video_frame->timestamp());

[emircan, 2016/07/12 22:57:00]

What if the coded_size() is not equal to visible_size()? These frames would have
the wrong visible_size() by default. 

[mcasas, 2016/07/13 00:45:30]

Well, I guess at this point the user is not going to 
get correct VideoFrames anyhow :)

+    } else {
+      frame = media::VideoFrame::CreateFrame(
+          media::PIXEL_FORMAT_I420, video_frame->coded_size(),
+          video_frame->visible_rect(), video_frame->natural_size(),
+          video_frame->timestamp());
+      const int result =
+          libyuv::ARGBToI420(video_frame->data(media::VideoFrame::kARGBPlane),
+                             video_frame->stride(media::VideoFrame::kARGBPlane),
+                             frame->data(media::VideoFrame::kYPlane),
+                             frame->stride(media::VideoFrame::kYPlane),
+                             frame->data(media::VideoFrame::kUPlane),
+                             frame->stride(media::VideoFrame::kUPlane),
+                             frame->data(media::VideoFrame::kVPlane),
+                             frame->stride(media::VideoFrame::kVPlane),
+                             video_frame->coded_size().width(),
+                             video_frame->coded_size().height());
+      DCHECK_EQ(0, result) << "Error converting ARGB input frame to I420";
+    }
+  } else if (video_frame->format() == media::PIXEL_FORMAT_YV12A) {
+    // Drop alpha channel since we do not support it yet.
     frame = media::WrapAsI420VideoFrame(video_frame);
+  } else {
+    frame = video_frame;
+  }
 
   encoding_task_runner_->PostTask(
       FROM_HERE, base::Bind(&Encoder::EncodeOnEncodingTaskRunner, this, frame,