From 9eb1ff3ac00fd3dce597fb2392309d424d6c2f38 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Thu, 17 Nov 2022 09:57:02 +0000 Subject: [PATCH] Revert "video_layer_allocation: clean up unused code" This reverts commit 05b58ad77e79efc5b4750f40b5092f945f0fff4d. Reason for revert: UB because the shift exponent (-2) is negative (UB happens at this line https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/rtp_rtcp/source/rtp_video_layers_allocation_extension.cc;l=279;drc=05b58ad77e79efc5b4750f40b5092f945f0fff4d). Original change's description: > video_layer_allocation: clean up unused code > > remove unused support for more than four spatial layer descriptions > of temporal layers > > BUG=webrtc:12000 > > Change-Id: I087bcd020897898636bdf9c838abafa8c73c53f3 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281320 > Reviewed-by: Tomas Gunnarsson > Commit-Queue: Philipp Hancke > Reviewed-by: Per Kjellander > Cr-Commit-Position: refs/heads/main@{#38646} Bug: webrtc:12000, webrtc:14678 Change-Id: Ib94a0dead98aeb84af9b91c0ca6ad0893e8f2874 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/283840 Auto-Submit: Mirko Bonadei Owners-Override: Mirko Bonadei Commit-Queue: Mirko Bonadei Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#38660} --- .../rtp_video_layers_allocation_extension.cc | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_video_layers_allocation_extension.cc b/modules/rtp_rtcp/source/rtp_video_layers_allocation_extension.cc index d44cb56205..5172ed4ce7 100644 --- a/modules/rtp_rtcp/source/rtp_video_layers_allocation_extension.cc +++ b/modules/rtp_rtcp/source/rtp_video_layers_allocation_extension.cc @@ -184,12 +184,16 @@ bool RtpVideoLayersAllocationExtension::Write( } ++write_at; - { // Number of temporal layers per spatial layer (at most kMaxSpatialIds). - static_assert(VideoLayersAllocation::kMaxSpatialIds == 4); + { // Number of temporal layers. int bit_offset = 8; *write_at = 0; for (const auto& layer : allocation.active_spatial_layers) { - bit_offset -= 2; + if (bit_offset == 0) { + bit_offset = 6; + *++write_at = 0; + } else { + bit_offset -= 2; + } *write_at |= ((layer.target_bitrate_per_temporal_layer.size() - 1) << bit_offset); } @@ -265,9 +269,8 @@ bool RtpVideoLayersAllocationExtension::Parse( return false; } - // Read number of temporal layers per spatial layer (at most kMaxSpatialIds), - // create `allocation->active_spatial_layers` while iterating though it. - static_assert(VideoLayersAllocation::kMaxSpatialIds == 4); + // Read number of temporal layers, + // Create `allocation->active_spatial_layers` while iterating though it. int bit_offset = 8; for (int stream_idx = 0; stream_idx < num_rtp_streams; ++stream_idx) { for (int sid = 0; sid < VideoLayersAllocation::kMaxSpatialIds; ++sid) { @@ -275,7 +278,14 @@ bool RtpVideoLayersAllocationExtension::Parse( continue; } - bit_offset -= 2; + if (bit_offset == 0) { + bit_offset = 6; + if (++read_at == end) { + return false; + } + } else { + bit_offset -= 2; + } int num_temporal_layers = 1 + ((*read_at >> bit_offset) & 0b11); allocation->active_spatial_layers.emplace_back(); auto& layer = allocation->active_spatial_layers.back();