Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282713002:
Introduce webrtc::VideoFrame::timestamp_us, and corresponding constructor. (Closed)
|
Created:
4 years, 3 months ago by nisse-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntroduce webrtc::VideoFrame::timestamp_us, and corresponding constructor.
Replaces render_time_ms_, but old accessors are kept for
compatibility.
Also short-circuit timestamp translation in
WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame.
BUG=webrtc:5682, webrtc:5740
Committed: https://crrev.com/74c10b5a7aee97e91dc533c430e0e6110c952b94
Cr-Commit-Position: refs/heads/master@{#14062}
Patch Set 1 #Patch Set 2 : Delete testcase WebRtcVideoEngine2Test.ProducesIncreasingTimestampsWithResetInputSources #
Total comments: 13
Patch Set 3 : Improve comments and constructor order. #
Total comments: 2
Patch Set 4 : Rename timestamp_ --> timestamp_rtp_ #
Messages
Total messages: 37 (18 generated)
The CQ bit was checked by nisse@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15858)
The CQ bit was checked by nisse@webrtc.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.webrtc.org/...
nisse@webrtc.org changed reviewers: + danilchap@webrtc.org, perkj@webrtc.org
https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1667: int64_t frame_delta_ms = frame.GetTimeStamp() / rtc::kNumNanosecsPerMillisec; A bonus point is that this deletes one of the few uses of GetTimeStamp.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15871)
looks good. some style comments https://codereview.webrtc.org/2282713002/diff/20001/webrtc/common_video/video... File webrtc/common_video/video_frame.cc (right): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/common_video/video... webrtc/common_video/video_frame.cc:26: VideoFrame::VideoFrame(const rtc::scoped_refptr<VideoFrameBuffer>& buffer, order constructor same as in header file, i.e. this one after the default constructor. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:32: // New style constructor, consistent with cricket::WebRtcVideoFrame. do not use word 'new' it will be old quickly. "Constructor consistent with cricket::WebRtcVideFrame" might be better comment if you plan to keep both constructors. No comment probably better otherwise. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:33: VideoFrame(const rtc::scoped_refptr<VideoFrameBuffer>& buffer, might be cleaner to pass this refptr by value instead of by const& (since it is moved into video_frame_buffer_) That would give class user choice - give a refcopy or move ptr<VideFrameBuffer>, where const& force copy and thus might add unnecessary AddRef/Release calls. e.g. black_buffer can be moved. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:34: webrtc::VideoRotation rotation, for consistency might be better to order parameters like members (and similar to next constructor), i.e. timestamp before rotation. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:37: // Preferred old style constructor. avoice using word 'old' too for same reason as word 'new'. If you plan to remove it, write deprecated.
lgtm with the Danils comments addressed. Have you tried git blame to see who wrote the test initially to figure out when the removed unit test code was triggered? https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1729: first_frame_timestamp_ms_ = rtc::Optional<int64_t>(); Are you saying that two different implementations of |source| now always must have the same time base and that they do in practice? Ie, switching from chrome camera - to screen cast and back to another camera? That is nice.
On 2016/08/26 15:38:53, perkj_webrtc wrote: > lgtm with the Danils comments addressed. Have you tried git blame to see who > wrote the test initially to figure out when the removed unit test code was > triggered? It seems the unit test and the epoch logic was added by pbos a year ago, in cl https://codereview.webrtc.org/1345473002
https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1729: first_frame_timestamp_ms_ = rtc::Optional<int64_t>(); On 2016/08/26 15:38:52, perkj_webrtc wrote: > Are you saying that two different implementations of |source| now always must > have the same time base and that they do in practice? I think so. Everything not using rtc::TimeMicros directly ought to use the TimestampAligner logic to translate. We could wait landing until after https://codereview.chromium.org/2270723002/ (Chrome), if you like. > Ie, switching from chrome camera - to screen cast and back to another camera? > That is nice. I think there could be time jumps on the order of a few ms, since capturer sources may have different delays between attaching the timestamp and getting here. But just dropping too early frames (which is done downstream) should handle that well enough, I think. If we think the black-frame logic will stay here, we could do the frame order check here as well, since both need the same last_frame_timestamp_us state. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:32: // New style constructor, consistent with cricket::WebRtcVideoFrame. On 2016/08/26 13:19:30, danilchap wrote: > do not use word 'new' > it will be old quickly. > "Constructor consistent with cricket::WebRtcVideFrame" > might be better comment if you plan to keep both constructors. > No comment probably better otherwise. I rephrased as a TODO comment pointing out the direction. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:33: VideoFrame(const rtc::scoped_refptr<VideoFrameBuffer>& buffer, On 2016/08/26 13:19:30, danilchap wrote: > might be cleaner to pass this refptr by value instead of by const& > (since it is moved into video_frame_buffer_) > > That would give class user choice - give a refcopy or move ptr<VideFrameBuffer>, > where const& force copy and thus might add unnecessary AddRef/Release calls. > e.g. black_buffer can be moved. Makes sense. But I think it's for a different cl, doing this change to all similar VideoFrame constructors. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:34: webrtc::VideoRotation rotation, On 2016/08/26 13:19:30, danilchap wrote: > for consistency might be better to order parameters like members (and similar to > next constructor), i.e. timestamp before rotation. Sorry about that. But this argument ordering is a workaround to get distinct signatures for the deprecated nanosec constructor and the new microsec constructor, on the cricket::WebRtcVideoFrame class. https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:37: // Preferred old style constructor. On 2016/08/26 13:19:30, danilchap wrote: > avoice using word 'old' too for same reason as word 'new'. > If you plan to remove it, write deprecated. Done.
lgtm https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/2282713002/diff/20001/webrtc/video_frame.h#newc... webrtc/video_frame.h:34: webrtc::VideoRotation rotation, On 2016/08/29 07:49:58, nisse-webrtc wrote: > On 2016/08/26 13:19:30, danilchap wrote: > > for consistency might be better to order parameters like members (and similar > to > > next constructor), i.e. timestamp before rotation. > > Sorry about that. But this argument ordering is a workaround to get distinct > signatures for the deprecated nanosec constructor and the new microsec > constructor, on the cricket::WebRtcVideoFrame class. Acknowledged. Not sure why consistency with WebRtcVideFrame matter.
On 2016/08/29 08:28:17, danilchap wrote: > Not sure why consistency with WebRtcVideFrame matter. Because we're aiming to delete it (and its base class cricket::VideoFrame), and in the transition make both aliases for webrtc::VideoFrame. And since I'm also working to delete VideoFrameFactory, the WebRtcVideoFrame constructor is the advertised way to create cricket::VideoFrame everywhere.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2282713002/#ps40001 (title: "Improve comments and constructor order.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, no build URL)
On 2016/08/29 08:48:25, nisse-webrtc wrote: > On 2016/08/29 08:28:17, danilchap wrote: > > > Not sure why consistency with WebRtcVideFrame matter. > > Because we're aiming to delete it (and its base class cricket::VideoFrame), and > in the transition make both aliases for webrtc::VideoFrame. > Alias! didn't know that part. Make sense then.
nisse@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman: PTAL, OWNER's approval needed for the changes to webrtc::VideoFrame.
LG for the change, but one comment regarding the RTP timestamp in the class. It would be great to get rid of that in the future, I think we're incorrectly using it in some places today where we rather should use the capture time, but it would be good to rename to maybe timestamp_rtp_ to really indicate what it is. WDYT? https://codereview.webrtc.org/2282713002/diff/40001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/2282713002/diff/40001/webrtc/video_frame.h#newc... webrtc/video_frame.h:190: uint32_t timestamp_; Can we rename this one for now, since this is used for RTP timestamp and is/can be different to the other time in this class.
https://codereview.webrtc.org/2282713002/diff/40001/webrtc/video_frame.h File webrtc/video_frame.h (right): https://codereview.webrtc.org/2282713002/diff/40001/webrtc/video_frame.h#newc... webrtc/video_frame.h:190: uint32_t timestamp_; On 2016/09/02 12:29:59, mflodman wrote: > Can we rename this one for now, since this is used for RTP timestamp and is/can > be different to the other time in this class. Renaming the member variable is easy. Renaming setter/getter methods, I don't know how much code would be affected. So what more precisely are you suggesting? Then there's also the transport_frame_id (introduced recently in cricket::VideoFrame, utility somewhat unclear to me). As far as I understand, that is also intended to hold the rtp timestamp, when we're using rtp.
Discussed offline, LGTM with variable renamed.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, perkj@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2282713002/#ps60001 (title: "Rename timestamp_ --> timestamp_rtp_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Introduce webrtc::VideoFrame::timestamp_us, and corresponding constructor. Replaces render_time_ms_, but old accessors are kept for compatibility. Also short-circuit timestamp translation in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame. BUG=webrtc:5682, webrtc:5740 ========== to ========== Introduce webrtc::VideoFrame::timestamp_us, and corresponding constructor. Replaces render_time_ms_, but old accessors are kept for compatibility. Also short-circuit timestamp translation in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame. BUG=webrtc:5682, webrtc:5740 Committed: https://crrev.com/74c10b5a7aee97e91dc533c430e0e6110c952b94 Cr-Commit-Position: refs/heads/master@{#14062} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/74c10b5a7aee97e91dc533c430e0e6110c952b94 Cr-Commit-Position: refs/heads/master@{#14062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
