Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939493002:
RTCVideoEncoder.SupportsNativeHandle() should return true (Closed)
|
Created:
3 years, 6 months ago by magjed_chromium Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRTCVideoEncoder.SupportsNativeHandle() should return true
RTCVideoEncoder.SupportsNativeHandle() means it can handle frames of
type kNative, which in Chromium represents wrapped media::VideoFrames.
Also, before accessing I420 data, i.e. DataY/U/V or StrideY/U/V, the
function ToI420 should be called on webrtc::VideoFrameBuffer (these
I420 functions will be removed from webrtc::VideoFrameBuffer soon).
BUG=732345, 732418
Review-Url: https://codereview.chromium.org/2939493002
Cr-Commit-Position: refs/heads/master@{#478769}
Committed: https://chromium.googlesource.com/chromium/src/+/927ddf0ccfa6947292f0551c83e427822ac18045
Patch Set 1 #
Total comments: 4
Messages
Total messages: 19 (12 generated)
The CQ bit was checked by magjed@chromium.org to run a CQ dry run
Patchset #1 (id:1) 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...
Description was changed from ========== RTCVideoEncoder.SupportsNativeHandle() should return true RTCVideoEncoder.SupportsNativeHandle() means it can handle frames of type kNative, which in Chromium represents wrapped media::VideoFrames. BUG=732345,732418 ========== to ========== RTCVideoEncoder.SupportsNativeHandle() should return true RTCVideoEncoder.SupportsNativeHandle() means it can handle frames of type kNative, which in Chromium represents wrapped media::VideoFrames. Also, before accessing I420 data, i.e. DataY/U/V or StrideY/U/V, the function ToI420 should be called on webrtc::VideoFrameBuffer (these I420 functions will be removed from webrtc::VideoFrameBuffer soon). BUG=732345,732418 ==========
magjed@chromium.org changed reviewers: + emircan@chromium.org
emircan@ - please take a look.
emircan@chromium.org changed reviewers: + nisse@webrtc.org
https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:623: : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()); We were hitting DCHECK before because next_frame->ntp_time_ms() was 0. We should still handle this case if webrtc makes a copy and drops the media::VideoFrame reference somehow, i.e. simulcast was doing that before. I am not sure what to use here, maybe nisse@ can help. We need to input a strictly increasing presentation timestamp to HW encoders. next_frame->timestamp() is in 90 kHz; we can't use here and 90 kHz domain is why we need to map back and forth.
https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:623: : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()); On 2017/06/12 20:06:16, emircan wrote: > We were hitting DCHECK before because next_frame->ntp_time_ms() was 0. We should > still handle this case if webrtc makes a copy and drops the media::VideoFrame > reference somehow, i.e. simulcast was doing that before. > > I am not sure what to use here, maybe nisse@ can help. We need to input a > strictly increasing presentation timestamp to HW encoders. > next_frame->timestamp() is in 90 kHz; we can't use here and 90 kHz domain is why > we need to map back and forth. Can we land this CL to fix the immediate issues and discuss the timestamp issue in a follow-up CL?
On 2017/06/12 20:28:14, magjed_chromium wrote: > https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... > File content/renderer/media/gpu/rtc_video_encoder.cc (right): > > https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... > content/renderer/media/gpu/rtc_video_encoder.cc:623: : > base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()); > On 2017/06/12 20:06:16, emircan wrote: > > We were hitting DCHECK before because next_frame->ntp_time_ms() was 0. We > should > > still handle this case if webrtc makes a copy and drops the media::VideoFrame > > reference somehow, i.e. simulcast was doing that before. > > > > I am not sure what to use here, maybe nisse@ can help. We need to input a > > strictly increasing presentation timestamp to HW encoders. > > next_frame->timestamp() is in 90 kHz; we can't use here and 90 kHz domain is > why > > we need to map back and forth. > > Can we land this CL to fix the immediate issues and discuss the timestamp issue > in a follow-up CL? Sure. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497302041099540,
"parent_rev": "8c640b21a3e2c560e33a5a2f4b186b87fa40301b", "commit_rev":
"927ddf0ccfa6947292f0551c83e427822ac18045"}
Message was sent while issue was closed.
Description was changed from ========== RTCVideoEncoder.SupportsNativeHandle() should return true RTCVideoEncoder.SupportsNativeHandle() means it can handle frames of type kNative, which in Chromium represents wrapped media::VideoFrames. Also, before accessing I420 data, i.e. DataY/U/V or StrideY/U/V, the function ToI420 should be called on webrtc::VideoFrameBuffer (these I420 functions will be removed from webrtc::VideoFrameBuffer soon). BUG=732345,732418 ========== to ========== RTCVideoEncoder.SupportsNativeHandle() should return true RTCVideoEncoder.SupportsNativeHandle() means it can handle frames of type kNative, which in Chromium represents wrapped media::VideoFrames. Also, before accessing I420 data, i.e. DataY/U/V or StrideY/U/V, the function ToI420 should be called on webrtc::VideoFrameBuffer (these I420 functions will be removed from webrtc::VideoFrameBuffer soon). BUG=732345,732418 Review-Url: https://codereview.chromium.org/2939493002 Cr-Commit-Position: refs/heads/master@{#478769} Committed: https://chromium.googlesource.com/chromium/src/+/927ddf0ccfa6947292f0551c83e4... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001) as https://chromium.googlesource.com/chromium/src/+/927ddf0ccfa6947292f0551c83e4...
Message was sent while issue was closed.
nisse@chromium.org changed reviewers: + nisse@chromium.org
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... File content/renderer/media/gpu/rtc_video_encoder.cc (right): https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:623: : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()); On 2017/06/12 20:06:16, emircan wrote: > We were hitting DCHECK before because next_frame->ntp_time_ms() was 0. We should > still handle this case if webrtc makes a copy and drops the media::VideoFrame > reference somehow, i.e. simulcast was doing that before. > > I am not sure what to use here, maybe nisse@ can help. We need to input a > strictly increasing presentation timestamp to HW encoders. > next_frame->timestamp() is in 90 kHz; we can't use here and 90 kHz domain is why > we need to map back and forth. I don't quite understand the context here, but in general, the timestamp_us() method should give you a valid and increasing timestamp. https://codereview.chromium.org/2939493002/diff/20001/content/renderer/media/... content/renderer/media/gpu/rtc_video_encoder.cc:640: rtc::scoped_refptr<webrtc::I420BufferInterface> i420_buffer = Do we intend to change ToI420 to a const method? If so, could use rtc::scoped_refptr<const webrtc::I420BufferInterface> here, right away. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
