Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2147753002:
VideoTrackRecorder: add support for texture backed ARGB VideoFrames (Closed)
|
Created:
4 years, 5 months ago by mcasas Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, mlamouri+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVideoTrackRecorder: add support for texture backed ARGB VideoFrames
Certain accelerated video decoders (in Android and CrOs, see bug)
produce ARGB VideoFrames that are backed by textures, which are
not supported on ToT by VideoTrackRecorder, so nothing gets encoded.
This CL adds code that borrows from webmediaplayer_ms_compositor.cc's
CopyFrame() [1] to try and retrieve the data from the said incoming
VideoFrame, by passing the this to the new method
VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread()
Thanks to this, content_browsertest's MediaRecorderPeerConnection
can be enabled.
[1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms_compositor.cc?dr=C&q=webmediaplayer_ms_compositor&sq=package:chromium&l=35
BUG=585242
TEST=the newly enabled content_browsertests and
manual testing on N6, see bug for details.
Committed: https://crrev.com/e641a694896375b8a8137d0d515dd975d132c3ad
Cr-Commit-Position: refs/heads/master@{#405867}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Rebased. Using RetrieveFrameOnMainThread() for texture-backed frames. #
Total comments: 6
Patch Set 3 : emircan@s comments #
Messages
Total messages: 27 (11 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== VideoTrackRecorder: add support for incoming ARGB VideoFrames, record black when not mappable. Certain Android video decoders (see bug) will inject ARGB VideoFrames into MediaStreams. This CL adds code so that: - if the incoming VideoFrame is ARGB, then it's converted to I420; the I420 version is encoded as usual - if the incoming VideoFrame is ARGB but is not IsMappable() (e.g. if IsMappable()==false, or HasTextures()), then black frames of the appropriate resolution are inserted instead. Also, two new VideoTrackRecorderTest test cases are added for these two new cases: - EncodingARGBFrame - EncodingNonMappableFrame BUG=585242 ========== to ========== VideoTrackRecorder: add support for incoming ARGB VideoFrames, record black when not mappable. Certain Android video decoders (see bug) will inject ARGB VideoFrames into MediaStreams, and they are not supported on VideoTrackRecoder::Encoder. This CL adds code so that: - if the incoming VideoFrame is ARGB, then it's converted to I420; this I420 version is encoded as usual - if the incoming VideoFrame is ARGB but is not IsMappable() (e.g. if IsMappable()==false, or HasTextures()), then black frames of the appropriate resolution are inserted instead. Also, two new VideoTrackRecorderTest test cases are added for these two new cases: - EncodingARGBFrame - EncodingNonMappableFrame BUG=585242 ==========
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ PTAL
https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:192: } 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... https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:200: video_frame->coded_size(), 0u, 0x80, 0x80, video_frame->timestamp()); What if the coded_size() is not equal to visible_size()? These frames would have the wrong visible_size() by default. https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder_unittest.cc:255: const uint8_t* y_plane = encoded_frame->data(media::VideoFrame::kYPlane); Can we have this in a for loop?
https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:192: } On 2016/07/12 22:57:00, emircan wrote: > This code looks very similar to CopyFrame() below, which handles texture > download as well. Can we refactor these into a common place? 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? > > 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. > 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. > > Seeing lots of black frames may confuse the developer as well. 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. > 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... https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:200: video_frame->coded_size(), 0u, 0x80, 0x80, video_frame->timestamp()); On 2016/07/12 22:57:00, emircan wrote: > What if the coded_size() is not equal to visible_size()? These frames would have > the wrong visible_size() by default. Well, I guess at this point the user is not going to get correct VideoFrames anyhow :) https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder_unittest.cc:255: const uint8_t* y_plane = encoded_frame->data(media::VideoFrame::kYPlane); On 2016/07/12 22:57:00, emircan wrote: > Can we have this in a for loop? Do you mean: for (int i = 0; i < y_plane_size; ++i) EXPECT_EQ(0u, encoded_frame->data(media::VideoFrame::kYPlane)[i]); Wouldn't that evaluate a function call every loop iteration? FTR, there is {ASSERT,EXPECT}_THAT(Collection, Each(Eq(kValue))); but that only works with types having iterators. So sad.
On 2016/07/13 00:45:31, mcasas wrote: > 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. Eventually, we would have to use a similar solution to CopyFrame() to download the textures, right? I don't see sending black frames as a permanent solution here. What I am suggesting is solving it in this patch and not introduce black frames at all. Moreover, the problem here is the HW decoders, and all HW decoders would output texture on all platforms(i.e. remote streams, <video> playback), so scope of this issue is probably not just Android. WDYT? What are your plans? As far as I understand, SkCanvasVideoRenderer can be initialized easily as it has the default ctor. SkCanvasVideoRenderer::Copy() needs the context provider, which should be easily accessible via main render thread: https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... I know these initializations might be redundant if there is no texture backed frame incoming. However, VideoTrackRecorder::InitializeEncoder() knows the first frame, so we can initialize based on that? > 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. 1) If Android MediaCodec can encode textures directly, we can just send them via VEAEncoder. VideoTrackRecorder::InitializeEncoder() can decide that based on the first frame. Still we need to verify that. 2) I see your point about black frames vs. nothing. Let me know if you still wanna continue with this approach. > > Well, I guess at this point the user is not going to > get correct VideoFrames anyhow :) They can at least have the right size. You can just make black frames with visible_rect() size. > https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... > content/renderer/media/video_track_recorder_unittest.cc:255: const uint8_t* > y_plane = encoded_frame->data(media::VideoFrame::kYPlane); > > Do you mean: > > for (int i = 0; i < y_plane_size; ++i) > EXPECT_EQ(0u, encoded_frame->data(media::VideoFrame::kYPlane)[i]); > > Wouldn't that evaluate a function call every loop iteration? > > FTR, there is > {ASSERT,EXPECT}_THAT(Collection, Each(Eq(kValue))); > but that only works with types having iterators. So sad. I was referring to: for (int i = 0; i <= media::VideoFrame::kVPlane; ++i) { const uint8_t* plane = encoded_frame->data(i); const int plane_size = encoded_frame->row_bytes(i); ... }
Patchset #2 (id:40001) has been deleted
Description was changed from ========== VideoTrackRecorder: add support for incoming ARGB VideoFrames, record black when not mappable. Certain Android video decoders (see bug) will inject ARGB VideoFrames into MediaStreams, and they are not supported on VideoTrackRecoder::Encoder. This CL adds code so that: - if the incoming VideoFrame is ARGB, then it's converted to I420; this I420 version is encoded as usual - if the incoming VideoFrame is ARGB but is not IsMappable() (e.g. if IsMappable()==false, or HasTextures()), then black frames of the appropriate resolution are inserted instead. Also, two new VideoTrackRecorderTest test cases are added for these two new cases: - EncodingARGBFrame - EncodingNonMappableFrame BUG=585242 ========== to ========== VideoTrackRecorder: add support for texture backed ARGB VideoFrames Certain accelerated video decoders (in Android and CrOs, see bug) produce ARGB VideoFrames that are backed by textures, which are not supported on ToT by VideoTrackRecorder, so nothing gets encoded. This CL adds code that borrows from webmediaplayer_ms_compositor.cc's CopyFrame() [1] to try and retrieve the data from the said incoming VideoFrame, by passing the this to the new method VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread() Thanks to this, content_browsertest's MediaRecorderPeerConnection can be enabled. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... BUG=585242 TEST=the newly enabled content_browsertests and manual testing on N6, see bug for details. ==========
Patchset #2 (id:60001) has been deleted
On 2016/07/14 02:55:55, emircan wrote: > On 2016/07/13 00:45:31, mcasas wrote: > > 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. > > Eventually, we would have to use a similar solution to CopyFrame() to download > the textures, right? I don't see sending black frames as a permanent solution > here. What I am suggesting is solving it in this patch and not introduce black > frames at all. Moreover, the problem here is the HW decoders, and all HW > decoders would output texture on all platforms(i.e. remote streams, <video> > playback), so scope of this issue is probably not just Android. WDYT? What are > your plans? > > As far as I understand, SkCanvasVideoRenderer can be initialized easily as it > has the default ctor. SkCanvasVideoRenderer::Copy() needs the context provider, > which should be easily accessible via main render thread: > https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... > I know these initializations might be redundant if there is no texture backed > frame incoming. However, VideoTrackRecorder::InitializeEncoder() knows the first > frame, so we can initialize based on that? > Thanks for the suggestion and clarification! PS2 makes use of SkCanvasVideoRenderer and draws heavily on CopyFrame() [1], however, I didn't refactor that one out due to a couple of issues: - Copy() function has two code paths, of which only the first one if(frame->HasTextures()) applies in this case, the second path makes a deep copy of the VideoFrame. - Copy() uses the deprecated SkBitmap. I'm using the new fancy sk_sp<SkSurface>. I can refactor Copy() to use the new data types in another CL if that's OK. - Copy() has a bug! It uses libyuv::ARGBToI420, when it should take into account the ABGR versus ARGB issue on Android. Again, this would be corrected on another CL. (How is that code tested?). With all that, essentially only a handful of lines were reusable, so I considered it not interesting. Also, note that the SkCanvasRenderer and SkSurface are only initialized on-demand. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... > > 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. > > 1) If Android MediaCodec can encode textures directly, we can just send them via > VEAEncoder. VideoTrackRecorder::InitializeEncoder() can decide that based on the > first frame. Still we need to verify that. > 2) I see your point about black frames vs. nothing. Let me know if you still > wanna continue with this approach. > > > > Well, I guess at this point the user is not going to > > get correct VideoFrames anyhow :) > They can at least have the right size. You can just make black frames with > visible_rect() size. > > > > https://codereview.chromium.org/2147753002/diff/20001/content/renderer/media/... > > content/renderer/media/video_track_recorder_unittest.cc:255: const uint8_t* > > y_plane = encoded_frame->data(media::VideoFrame::kYPlane); > > > > Do you mean: > > > > for (int i = 0; i < y_plane_size; ++i) > > EXPECT_EQ(0u, encoded_frame->data(media::VideoFrame::kYPlane)[i]); > > > > Wouldn't that evaluate a function call every loop iteration? > > > > FTR, there is > > {ASSERT,EXPECT}_THAT(Collection, Each(Eq(kValue))); > > but that only works with types having iterators. So sad. > I was referring to: > for (int i = 0; i <= media::VideoFrame::kVPlane; ++i) { > const uint8_t* plane = encoded_frame->data(i); > const int plane_size = encoded_frame->row_bytes(i); > ... > }
emircan@ PTAL. Please start afresh in PS2, since plenty of lines of code has changed.
On 2016/07/15 02:16:21, mcasas wrote: > emircan@ PTAL. > > Please start afresh in PS2, since plenty of lines > of code have changed. FTR I wrote a bug on the necessary actions on CopyFrame(): https://crbug.com/628466
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:243: video_frame->coded_size(), 0u, 0x80, 0x80, video_frame->timestamp()); Replace coded_size() with visible_rect() here. CreateColorFrame() takes only one param, so keeping visible_rect() the same would be a better option. https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:291: frame->coded_size().width(), Replace coded_size() with visible_rect() here. You allocated SkPixmap with visible_rect on l.256 and copying into visible_data(). https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:293: libyuv::kRotate0, source_pixel_format) != 0) { Consider the case for rotation. We have seen that remote Android peer with H264 result in textures with rotation [0]. If that CL gets committed earlier, you can use the frame metadata. [0] https://codereview.chromium.org/2150203002/
emircan@ PTAL https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:243: video_frame->coded_size(), 0u, 0x80, 0x80, video_frame->timestamp()); On 2016/07/15 17:47:34, emircan wrote: > Replace coded_size() with visible_rect() here. CreateColorFrame() takes only one > param, so keeping visible_rect() the same would be a better option. Done. https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:291: frame->coded_size().width(), On 2016/07/15 17:47:34, emircan wrote: > Replace coded_size() with visible_rect() here. You allocated SkPixmap with > visible_rect on l.256 and copying into visible_data(). Done. https://codereview.chromium.org/2147753002/diff/100001/content/renderer/media... content/renderer/media/video_track_recorder.cc:293: libyuv::kRotate0, source_pixel_format) != 0) { On 2016/07/15 17:47:34, emircan wrote: > Consider the case for rotation. We have seen that remote Android peer with H264 > result in textures with rotation [0]. If that CL gets committed earlier, you can > use the frame metadata. > > [0] https://codereview.chromium.org/2150203002/ Added TODO, will revisit.
lgtm
mcasas@chromium.org changed reviewers: + hubbe@chromium.org
hubbe@ RS enabling test in content/browser/media
lgtm
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== VideoTrackRecorder: add support for texture backed ARGB VideoFrames Certain accelerated video decoders (in Android and CrOs, see bug) produce ARGB VideoFrames that are backed by textures, which are not supported on ToT by VideoTrackRecorder, so nothing gets encoded. This CL adds code that borrows from webmediaplayer_ms_compositor.cc's CopyFrame() [1] to try and retrieve the data from the said incoming VideoFrame, by passing the this to the new method VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread() Thanks to this, content_browsertest's MediaRecorderPeerConnection can be enabled. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... BUG=585242 TEST=the newly enabled content_browsertests and manual testing on N6, see bug for details. ========== to ========== VideoTrackRecorder: add support for texture backed ARGB VideoFrames Certain accelerated video decoders (in Android and CrOs, see bug) produce ARGB VideoFrames that are backed by textures, which are not supported on ToT by VideoTrackRecorder, so nothing gets encoded. This CL adds code that borrows from webmediaplayer_ms_compositor.cc's CopyFrame() [1] to try and retrieve the data from the said incoming VideoFrame, by passing the this to the new method VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread() Thanks to this, content_browsertest's MediaRecorderPeerConnection can be enabled. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... BUG=585242 TEST=the newly enabled content_browsertests and manual testing on N6, see bug for details. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== VideoTrackRecorder: add support for texture backed ARGB VideoFrames Certain accelerated video decoders (in Android and CrOs, see bug) produce ARGB VideoFrames that are backed by textures, which are not supported on ToT by VideoTrackRecorder, so nothing gets encoded. This CL adds code that borrows from webmediaplayer_ms_compositor.cc's CopyFrame() [1] to try and retrieve the data from the said incoming VideoFrame, by passing the this to the new method VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread() Thanks to this, content_browsertest's MediaRecorderPeerConnection can be enabled. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... BUG=585242 TEST=the newly enabled content_browsertests and manual testing on N6, see bug for details. ========== to ========== VideoTrackRecorder: add support for texture backed ARGB VideoFrames Certain accelerated video decoders (in Android and CrOs, see bug) produce ARGB VideoFrames that are backed by textures, which are not supported on ToT by VideoTrackRecorder, so nothing gets encoded. This CL adds code that borrows from webmediaplayer_ms_compositor.cc's CopyFrame() [1] to try and retrieve the data from the said incoming VideoFrame, by passing the this to the new method VideoTrackRecorder::Encoder::RetrieveFrameOnMainThread() Thanks to this, content_browsertest's MediaRecorderPeerConnection can be enabled. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms... BUG=585242 TEST=the newly enabled content_browsertests and manual testing on N6, see bug for details. Committed: https://crrev.com/e641a694896375b8a8137d0d515dd975d132c3ad Cr-Commit-Position: refs/heads/master@{#405867} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e641a694896375b8a8137d0d515dd975d132c3ad Cr-Commit-Position: refs/heads/master@{#405867} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
