|
|
Created:
4 years, 1 month ago by emircan Modified:
4 years, 1 month ago Reviewers:
mcasas CC:
chromium-reviews, mlamouri+watch-content_chromium.org, dominickn+watch_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, mac-reviews_chromium.org, piman+watch_chromium.org, James Su, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd callback to copy texture backed frames in WebRtcVideoFrameAdapter
This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter
so that hardware decoded video tracks can be cloned or forwarded. The callback is
assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread.
BUG=642663
TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac.
Committed: https://crrev.com/e0a575d620207471068e04b133f7e24ff84b23bd
Cr-Commit-Position: refs/heads/master@{#428535}
Patch Set 1 #
Total comments: 18
Patch Set 2 : mcasas@ comments. #Patch Set 3 : Rebase. #Patch Set 4 : Fix asan. #
Total comments: 4
Patch Set 5 : mcasas@ comments. #
Messages
Total messages: 35 (27 generated)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== down tex use map instead BUG= ========== to ========== Add callback to copy texture backed frames in WebRtcVideoFrameAdapter This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter so that hardware decoded video tracks can be cloned or forwarded. The callback is assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread. BUG=642663 TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org
Patchset #1 (id:1) has been deleted
PTAL.
looking good, I've got a few suggestions. Bots please. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:48: DVLOG(3) << " WebRtcVideoCapturerAdapter::dtor"; unrelated nit: I'm trying to correct these statements to DVLO(x) << __func__; https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:64: << " format: " << input_frame->format(); These are going to print numbers, right? Consider using media::VIdeoCaptureFormat::ToString(). https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:189: DCHECK(!fourccs || fourccs->empty()); nit: not your code, but here we should not DCHECK() a case we're handling later. Suggestion for this method: if (!fourccs) return false; DCHECK(fourccs->empty()); fourcss->push_back(cricket::FOURCC_I420); return true; https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:205: << " h = " << desired.height; nit: DVLOG(3) << __func__ << " desired: " << desired.ToString(); here and in l.165; https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:225: base::Unretained(this), frame, new_frame, &waiter)); weak_factory_.GetWeakPtr() ? https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:254: frame->visible_rect().size(), 0u, 0x80, 0x80, frame->timestamp()); Shouldn't you return here (and in l.259)? Ah but then |waiter| would not be signalled. I looked around but there is no ScopedWaitableEvent that would Signal on destruction. Consider adding a class class ScopedWaitableEvent { public: explicit ScopedWaitableEvent(base::WaitableEvent* event) : event_(event) {} ~ScopedWaitableEvent() { if (event_) event_->Wait(); } private: base::WaitableEvent* const event_; }; using it to wrap |waiter| in l.239 after a DCHECK(waiter), and then just return as usual in every error path, removing l.277-278, WDYT? https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:278: waiter->Signal(); Is there a chance that |waiter| might be nullptr? if not, just DCHECK() it (otherwise we would not crash, but playback would hold up). Edit: forget this in light of my comment in l.254. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:68: base::WaitableEvent* waiter); I see with worry the existence of |base::WaitableEvent| but I guess it's necessary and inevitable. Could you plz add a comment in these methods explaining that CopyTextureFrame() is going to block until the copy on Main Thread is finished? https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_frame_adapter.h (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_frame_adapter.h:50: scoped_refptr<media::VideoFrame> frame_; nit: IINM this might be const as well
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:48: DVLOG(3) << " WebRtcVideoCapturerAdapter::dtor"; On 2016/10/26 16:06:02, mcasas wrote: > unrelated nit: I'm trying to correct these statements to > DVLO(x) << __func__; Done. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:64: << " format: " << input_frame->format(); On 2016/10/26 16:06:02, mcasas wrote: > These are going to print numbers, right? > Consider using media::VIdeoCaptureFormat::ToString(). Done. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:189: DCHECK(!fourccs || fourccs->empty()); On 2016/10/26 16:06:02, mcasas wrote: > nit: not your code, but here we should not DCHECK() a > case we're handling later. Suggestion for this method: > > if (!fourccs) > return false; > DCHECK(fourccs->empty()); > fourcss->push_back(cricket::FOURCC_I420); > return true; Done. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:205: << " h = " << desired.height; On 2016/10/26 16:06:02, mcasas wrote: > nit: > DVLOG(3) << __func__ << " desired: " << desired.ToString(); > > here and in l.165; Done. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:225: base::Unretained(this), frame, new_frame, &waiter)); On 2016/10/26 16:06:02, mcasas wrote: > weak_factory_.GetWeakPtr() ? I can add a WeakPtr here to be used on main thread and invalidate on it. But then, I cannot use the weakptr for the |copy_texture_callback_| that runs on some rtc platform thread or invalidate. I thought again about the design and decided to extract these copy methods to another class TextureCopier which will be ref counted. That way we can pass the class to callbacks safely. Let me know if you would prefer to move it to a seperate file and/or directory. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:254: frame->visible_rect().size(), 0u, 0x80, 0x80, frame->timestamp()); On 2016/10/26 16:06:02, mcasas wrote: > Shouldn't you return here (and in l.259)? Ah but then |waiter| > would not be signalled. I looked around but there is no > ScopedWaitableEvent that would Signal on destruction. > Consider adding a class > > class ScopedWaitableEvent { > public: > explicit ScopedWaitableEvent(base::WaitableEvent* event) : event_(event) {} > ~ScopedWaitableEvent() { > if (event_) > event_->Wait(); > } > > private: > base::WaitableEvent* const event_; > }; > > using it to wrap |waiter| in l.239 after a > DCHECK(waiter), and then just return as usual in every > error path, removing l.277-278, WDYT? Thanks. I added it to this file and restructured the conditionals accordingly. It would be better to move it to base, but I came across to a lot of opposition when trying to add to base earlier :( https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:278: waiter->Signal(); On 2016/10/26 16:06:02, mcasas wrote: > Is there a chance that |waiter| might be nullptr? > if not, just DCHECK() it (otherwise we would not > crash, but playback would hold up). > Edit: forget this in light of my comment in l.254. Done. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:68: base::WaitableEvent* waiter); On 2016/10/26 16:06:02, mcasas wrote: > I see with worry the existence of |base::WaitableEvent| > but I guess it's necessary and inevitable. Could you plz > add a comment in these methods explaining that > CopyTextureFrame() is going to block until the copy > on Main Thread is finished? I added comments explaining it. Unfortunately, I couldn't find a way to do it anywhere else than main thread for texture copy. https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_frame_adapter.h (right): https://codereview.chromium.org/2456443002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_frame_adapter.h:50: scoped_refptr<media::VideoFrame> frame_; On 2016/10/26 16:06:02, mcasas wrote: > nit: IINM this might be const as well I cant change it as it can be modified, see l.93 in cc file.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm with a few minor suggestions. https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:172: << media::VideoPixelFormatToString(input_frame->format()); nit: my suggestion was ToString() the whole thing, but if you stick to this, I suggest: DLOG(ERROR) << "Unsupported storage type: " << media::VideoCaptureFormat::PixelStorageToString( input_frame->storage_type()) << " for pixel format " << media::VideoPixelFormatToString(input_frame->format()); a bit crazy, huh? Instead perhaps just say: DLOG(ERROR) << "Unsupported storage type for incoming VideoFrame " << input_frame->AsHumanReadableString(); https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:267: webrtc::kVideoRotation_0, translated_camera_time_us), nit: Since here and in l.237 we know that frame->HasTextures() == false, should we still send a meaningful |copy_texture_callback|, or just an empty one instead?
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by emircan@chromium.org
https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:172: << media::VideoPixelFormatToString(input_frame->format()); On 2016/10/28 19:49:35, mcasas wrote: > nit: my suggestion was ToString() the whole thing, > but if you stick to this, I suggest: > > DLOG(ERROR) << "Unsupported storage type: " > << media::VideoCaptureFormat::PixelStorageToString( > input_frame->storage_type()) > << " for pixel format " > << media::VideoPixelFormatToString(input_frame->format()); > > a bit crazy, huh? Instead perhaps just say: > > DLOG(ERROR) << "Unsupported storage type for incoming VideoFrame " > << input_frame->AsHumanReadableString(); AsHumanReadableString() is awesome. Thanks. https://codereview.chromium.org/2456443002/diff/100001/content/renderer/media... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:267: webrtc::kVideoRotation_0, translated_camera_time_us), On 2016/10/28 19:49:35, mcasas wrote: > nit: Since here and in l.237 we know that > frame->HasTextures() == false, should we still > send a meaningful |copy_texture_callback|, or > just an empty one instead? Empty callback makes more sense as reference is held with scoped_refptr. Changing it.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2456443002/#ps120001 (title: "mcasas@ comments.")
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 ========== Add callback to copy texture backed frames in WebRtcVideoFrameAdapter This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter so that hardware decoded video tracks can be cloned or forwarded. The callback is assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread. BUG=642663 TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac. ========== to ========== Add callback to copy texture backed frames in WebRtcVideoFrameAdapter This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter so that hardware decoded video tracks can be cloned or forwarded. The callback is assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread. BUG=642663 TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add callback to copy texture backed frames in WebRtcVideoFrameAdapter This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter so that hardware decoded video tracks can be cloned or forwarded. The callback is assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread. BUG=642663 TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac. ========== to ========== Add callback to copy texture backed frames in WebRtcVideoFrameAdapter This CL adds a callback to copy texture backed frames in WebRtcVideoFrameAdapter so that hardware decoded video tracks can be cloned or forwarded. The callback is assigned by WebRtcVideoCapturerAdapter and runs in main renderer thread. BUG=642663 TEST=Ran https://loopback-dot-apprtc.appspot.com/?debug=loopback&vsc=h264 on Mac. Committed: https://crrev.com/e0a575d620207471068e04b133f7e24ff84b23bd Cr-Commit-Position: refs/heads/master@{#428535} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e0a575d620207471068e04b133f7e24ff84b23bd Cr-Commit-Position: refs/heads/master@{#428535} |