Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1656233002:
Avoid undefined behavior in vp8 screenshare_layers (Closed)
|
Created:
4 years, 10 months ago by sprang_webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid undefined behavior in vp8 screenshare_layers
active_layer_ could be dereferenced while being -1...
Also added som DCHECKs
BUG=webrtc:5490
Committed: https://crrev.com/2ddb8bd3593a2a2797fea16f358cc89c140f6154
Cr-Commit-Position: refs/heads/master@{#11486}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #Patch Set 3 : FrameEncoded can be called with active_layer -1 if num TL=1 #Patch Set 4 : Test error + brain fartin ConfigureBitrates #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by sprang@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656233002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656233002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
sprang@webrtc.org changed reviewers: + pbos@webrtc.org - sprang@google.com
lgtm with 2 comments https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right): https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:150: if (active_layer_ != -1 && Can you amend the comment above to include what -1 as active_layer_ means in this function? https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:224: if (active_layer_ != 1) { Can you DCHECK_NE here as well?
sprang@google.com changed reviewers: + sprang@google.com
https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc (right): https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:150: if (active_layer_ != -1 && On 2016/02/02 21:16:14, pbos-webrtc wrote: > Can you amend the comment above to include what -1 as active_layer_ means in > this function? Done. https://codereview.webrtc.org/1656233002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/vp8/screenshare_layers.cc:224: if (active_layer_ != 1) { On 2016/02/02 21:16:14, pbos-webrtc wrote: > Can you DCHECK_NE here as well? Done.
The CQ bit was checked by sprang@google.com
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/1656233002/#ps20001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656233002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656233002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12234) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3206)
The CQ bit was checked by sprang@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/1656233002/#ps60001 (title: "Test error + brain fartin ConfigureBitrates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656233002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3233)
sprang@webrtc.org changed reviewers: + stefan@webrtc.org - sprang@google.com
+stefan as owner
Description was changed from ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG= ========== to ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG=webrtc:5490 ==========
On 2016/02/04 10:06:06, språng wrote: > +stefan as owner lgtm
The CQ bit was checked by sprang@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656233002/60001
Message was sent while issue was closed.
Description was changed from ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG=webrtc:5490 ========== to ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG=webrtc:5490 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG=webrtc:5490 ========== to ========== Avoid undefined behavior in vp8 screenshare_layers active_layer_ could be dereferenced while being -1... Also added som DCHECKs BUG=webrtc:5490 Committed: https://crrev.com/2ddb8bd3593a2a2797fea16f358cc89c140f6154 Cr-Commit-Position: refs/heads/master@{#11486} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2ddb8bd3593a2a2797fea16f358cc89c140f6154 Cr-Commit-Position: refs/heads/master@{#11486} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
