Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1688643003:
Initial cleanup of cricket::VideoFrame. (Closed)
|
Created:
4 years, 10 months ago by nisse-webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionInitial cleanup of cricket::VideoFrame.
Deleted GetRotation (old alias for GetVideoRotation).
Deleted CopyToBuffer.
Deleted Sizeof.
Deleted Write.
Demote CopyToPlanes to protected status.
BUG=webrtc:5426
Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9
Cr-Commit-Position: refs/heads/master@{#11564}
Committed: https://crrev.com/d73c99cf224d97b993dd938758679c075219acb0
Cr-Commit-Position: refs/heads/master@{#11594}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed pbos' comments. #Patch Set 3 : Rebase. #Patch Set 4 : Fix VideoCapturerTest::OnFrame again, efter it was resurrected by a revert. #Patch Set 5 : Rebase. #
Messages
Total messages: 44 (22 generated)
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org
Started with the low-hanging fruit. As for SizeOf, it makes sense to have a helper function somewhere to compute the size of a I420 frame, but I don't think it really belongs here.
lgtm, run git cl format if you haven't https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.h File webrtc/media/base/videoframe.h (right): https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.... webrtc/media/base/videoframe.h:80: virtual webrtc::VideoRotation GetVideoRotation() const { Can this be pure virtual now?
https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.h File webrtc/media/base/videoframe.h (right): https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.... webrtc/media/base/videoframe.h:80: virtual webrtc::VideoRotation GetVideoRotation() const { On 2016/02/10 15:15:32, pbos-webrtc wrote: > Can this be pure virtual now? At least the compiler doesn't complain. And WebRtcVideoFrame should be the only subclass, right?
On 2016/02/10 15:28:06, nisse-webrtc wrote: > https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.h > File webrtc/media/base/videoframe.h (right): > > https://codereview.webrtc.org/1688643003/diff/1/webrtc/media/base/videoframe.... > webrtc/media/base/videoframe.h:80: virtual webrtc::VideoRotation > GetVideoRotation() const { > On 2016/02/10 15:15:32, pbos-webrtc wrote: > > Can this be pure virtual now? > > At least the compiler doesn't complain. And WebRtcVideoFrame should be the only > subclass, right? If it builds in Chromium I think we're good.
pthatcher@google.com changed reviewers: + pthatcher@google.com
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1688643003/#ps20001 (title: "Addressed pbos' comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688643003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/733) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6966) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12645) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3366)
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1688643003/#ps40001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688643003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/40001
lgtm
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/1688953003/ by nisse@webrtc.org. The reason for reverting is: There appears there were some uses left of cricket::VideoFrame::GetRotation (to be replaced by GetVideoRotation). Investigating..
Message was sent while issue was closed.
Description was changed from ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 ========== to ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 ==========
On 2016/02/11 09:38:41, nisse-webrtc wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.webrtc.org/1688953003/ by mailto:nisse@webrtc.org. > > The reason for reverting is: There appears there were some uses left of > cricket::VideoFrame::GetRotation (to be replaced by GetVideoRotation). > Investigating.. There were compilation errors in VideoCaptureTest::OnFrame, a method that's being deleted and was dropped from this cl in the rebase. I think I'll just wait a little to let the dust settle, and then try relanding with no changes (but possibly with another rebase).
Message was sent while issue was closed.
Description was changed from ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 ========== to ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564}
Message was sent while issue was closed.
Description was changed from ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ========== to ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ==========
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, perkj@webrtc.org, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1688643003/#ps60001 (title: "Fix VideoCapturerTest::OnFrame again, efter it was resurrected by a revert.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688643003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/7003)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688643003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on 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/patch-status/1688643003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, perkj@webrtc.org, pthatcher@google.com Link to the patchset: https://codereview.webrtc.org/1688643003/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688643003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688643003/80001
Message was sent while issue was closed.
Description was changed from ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ========== to ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} ========== to ========== Initial cleanup of cricket::VideoFrame. Deleted GetRotation (old alias for GetVideoRotation). Deleted CopyToBuffer. Deleted Sizeof. Deleted Write. Demote CopyToPlanes to protected status. BUG=webrtc:5426 Committed: https://crrev.com/4d575b0d4276422bdf7b595d92c57c4f0f8ce0e9 Cr-Commit-Position: refs/heads/master@{#11564} Committed: https://crrev.com/d73c99cf224d97b993dd938758679c075219acb0 Cr-Commit-Position: refs/heads/master@{#11594} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d73c99cf224d97b993dd938758679c075219acb0 Cr-Commit-Position: refs/heads/master@{#11594} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
