Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751133002:
Remove dead code in vp8_impl files. (Closed)
|
Created:
3 years, 9 months ago by pbos-webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove dead code in vp8_impl files.
BUG=webrtc:7349
R=brandtr@webrtc.org, marpan@google.com, nisse@webrtc.org, marpan@webrtc.org
TBR=stefan@webrtc.org
Review-Url: https://codereview.webrtc.org/2751133002 .
Cr-Commit-Position: refs/heads/master@{#17324}
Committed: https://chromium.googlesource.com/external/webrtc/+/1182afedecc572718af4e68ba7cd594dc771456c
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : rebase #
Messages
Total messages: 23 (6 generated)
PTAL
pbos@webrtc.org changed reviewers: + stefan@webrtc.org
+stefan@, do we want to keep any of this (specifically feedback_mode_) ?
rebase
stefan@webrtc.org changed reviewers: + brandtr@webrtc.org, nisse@webrtc.org
nisse and rasmus should review this. nisse is doing similar clean up of sli and rpsi, and rasmus does codec stuff. :)
On 2017/03/16 08:53:55, stefan-webrtc wrote: > nisse and rasmus should review this. nisse is doing similar clean up of sli and > rpsi, and rasmus does codec stuff. :) My cl here: https://codereview.webrtc.org/2753783002/ Pending review of the simulcast test code which I don't understand. pbos, do you know what to do about the TestRPSI* tests?
On 2017/03/16 08:58:24, nisse-webrtc wrote: > On 2017/03/16 08:53:55, stefan-webrtc wrote: > > nisse and rasmus should review this. nisse is doing similar clean up of sli > and > > rpsi, and rasmus does codec stuff. :) > > My cl here: https://codereview.webrtc.org/2753783002/ > > Pending review of the simulcast test code which I don't understand. pbos, do you > know what to do about the TestRPSI* tests? marpan@ sure does, cc'd.
On 2017/03/16 16:14:08, pbos-webrtc wrote: > On 2017/03/16 08:58:24, nisse-webrtc wrote: > > On 2017/03/16 08:53:55, stefan-webrtc wrote: > > > nisse and rasmus should review this. nisse is doing similar clean up of sli > > and > > > rpsi, and rasmus does codec stuff. :) > > > > My cl here: https://codereview.webrtc.org/2753783002/ > > > > Pending review of the simulcast test code which I don't understand. pbos, do > you > > know what to do about the TestRPSI* tests? > > marpan@ sure does, cc'd. nisse@ is this CL mutually exclusive to yours or can I go ahead?
Nice cleanup! lgtm https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (left): https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h#old... webrtc/common_types.h:527: bool feedbackModeOn; Need to update in Chromium too: https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec...
lgtm https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (left): https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h#old... webrtc/common_types.h:527: bool feedbackModeOn; On 2017/03/17 08:26:13, brandtr wrote: > Need to update in Chromium too: > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... My plan was to first delete all use of this variable, then drop it in chrome, and finally delete it here. pictureLossIndicationOn is also unused, and could be deleted in chromium right away, and then deleted here. https://codereview.webrtc.org/2751133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/2751133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:741: int RPSI = codec_specific_info->codecSpecific.VP8.pictureIdRPSI; What's the state of rpsi (and sli) support after this change? Does the |rps_| become unused? If so, you can delete reference_picture_selection* too. (If not, I'll delete it later).
https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (left): https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h#old... webrtc/common_types.h:527: bool feedbackModeOn; On 2017/03/17 08:26:13, brandtr wrote: > Need to update in Chromium too: > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... Thanks, removing in https://codereview.chromium.org/2757913002/. https://codereview.webrtc.org/2751133002/diff/20001/webrtc/common_types.h#old... webrtc/common_types.h:527: bool feedbackModeOn; On 2017/03/17 08:54:48, nisse-webrtc wrote: > On 2017/03/17 08:26:13, brandtr wrote: > > Need to update in Chromium too: > > > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... > > > https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_dec... > > My plan was to first delete all use of this variable, then drop it in chrome, > and finally delete it here. pictureLossIndicationOn is also unused, and could be > deleted in chromium right away, and then deleted here. Ack, can do that separately though. https://codereview.webrtc.org/2751133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/2751133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:741: int RPSI = codec_specific_info->codecSpecific.VP8.pictureIdRPSI; On 2017/03/17 08:54:48, nisse-webrtc wrote: > What's the state of rpsi (and sli) support after this change? > > Does the |rps_| become unused? If so, you can delete > reference_picture_selection* too. (If not, I'll delete it later). Not sure, I can make sure to take a second pass after this CL, or you, whoever gets to it first. :)
rebase
marpan@google.com changed reviewers: + marpan@google.com
lgtm
On 2017/03/16 08:58:24, nisse-webrtc wrote: > On 2017/03/16 08:53:55, stefan-webrtc wrote: > > nisse and rasmus should review this. nisse is doing similar clean up of sli > and > > rpsi, and rasmus does codec stuff. :) > > My cl here: https://codereview.webrtc.org/2753783002/ Landed now, so you're going to get some delete/delete conflicts on rebase. At least that's the easiest kind of conflicts. My plan is to next rebase and land cl https://codereview.webrtc.org/2742383004/.
On 2017/03/21 09:02:04, nisse-webrtc wrote: > On 2017/03/16 08:58:24, nisse-webrtc wrote: > > On 2017/03/16 08:53:55, stefan-webrtc wrote: > > > nisse and rasmus should review this. nisse is doing similar clean up of sli > > and > > > rpsi, and rasmus does codec stuff. :) > > > > My cl here: https://codereview.webrtc.org/2753783002/ > > Landed now, so you're going to get some delete/delete conflicts on rebase. > At least that's the easiest kind of conflicts. > > My plan is to next rebase and land cl https://codereview.webrtc.org/2742383004/. Crap, I forgot to land this. :)
rebase
Description was changed from ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=marpan@webrtc.org ========== to ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=brandtr@webrtc.org, nisse@webrtc.org, marpan@webrtc.org ==========
Description was changed from ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=brandtr@webrtc.org, nisse@webrtc.org, marpan@webrtc.org ========== to ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=brandtr@webrtc.org, nisse@webrtc.org, marpan@webrtc.org TBR=stefan@webrtc.org ==========
+tbr stefan@ for common_types.h removal of feedbackModeOn.
Description was changed from ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=brandtr@webrtc.org, nisse@webrtc.org, marpan@webrtc.org TBR=stefan@webrtc.org ========== to ========== Remove dead code in vp8_impl files. BUG=webrtc:7349 R=brandtr@webrtc.org, marpan@google.com, nisse@webrtc.org, marpan@webrtc.org TBR=stefan@webrtc.org Review-Url: https://codereview.webrtc.org/2751133002 . Cr-Commit-Position: refs/heads/master@{#17324} Committed: https://chromium.googlesource.com/external/webrtc/+/1182afedecc572718af4e68ba... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 1182afedecc572718af4e68ba7cd594dc771456c (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
