Discussion:
[PATCH] libva-intel-driver: fix i965 encoder wrong bits shift operation
(too old to reply)
Sean V Kelley
2017-01-17 21:15:32 UTC
Permalink
From: Kuang-che Wu <***@chromium.org>

shift uint32_t by 32 bits is undefined behavior.

For this particular case: when invoke avc_bitstream_put_ui() with 32
bits value at byte position of multiple of 4, existing 32 bits garbage
data in the buffer may be retained instead of cleared. The result is,
the position of NALU start code (0x00000001) looks like overwritten by
garbage value.

Patch has been tested and used upstream:
https://chromium-review.googlesource.com/#/c/410541/

Signed-off-by: Kuang-che Wu <***@chromium.org>
Signed-off-by: Sean V Kelley <***@posteo.de>
---
src/i965_encoder_utils.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/i965_encoder_utils.c b/src/i965_encoder_utils.c
index ac58cd1a..e061d071 100644
--- a/src/i965_encoder_utils.c
+++ b/src/i965_encoder_utils.c
@@ -134,7 +134,11 @@ avc_bitstream_put_ui(avc_bitstream *bs, unsigned int val, int size_in_bits)
bs->buffer[pos] = (bs->buffer[pos] << size_in_bits | val);
} else {
size_in_bits -= bit_left;
- bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >> size_in_bits);
+ if (bit_left == 32) {
+ bs->buffer[pos] = (val >> size_in_bits);
+ } else {
+ bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >> size_in_bits);
+ }
bs->buffer[pos] = swap32(bs->buffer[pos]);

if (pos + 1 == bs->max_size_in_dword) {
--
2.11.0
Xiang, Haihao
2017-01-18 02:48:47 UTC
Permalink
Post by Sean V Kelley
shift uint32_t by 32 bits is undefined behavior.
For this particular case: when invoke avc_bitstream_put_ui() with 32
bits value at byte position of multiple of 4, existing 32 bits garbage
data in the buffer may be retained instead of cleared. The result is,
the position of NALU start code (0x00000001) looks like overwritten by
garbage value.
https://chromium-review.googlesource.com/#/c/410541/
---
 src/i965_encoder_utils.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/i965_encoder_utils.c b/src/i965_encoder_utils.c
index ac58cd1a..e061d071 100644
--- a/src/i965_encoder_utils.c
+++ b/src/i965_encoder_utils.c
@@ -134,7 +134,11 @@ avc_bitstream_put_ui(avc_bitstream *bs, unsigned int val, int size_in_bits)
         bs->buffer[pos] = (bs->buffer[pos] << size_in_bits | val);
     } else {
         size_in_bits -= bit_left;
-        bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >> size_in_bits);
+        if (bit_left == 32) {
+            bs->buffer[pos] = (val >> size_in_bits);
The input value of size_in_bits should be less than or equal to 32, so now the
value of size_in_bits is 0 for this case, I think  "bs->buffer[pos] = val" is
more clear.
Post by Sean V Kelley
+        } else {
+            bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >> size_in_bits);
+        }
         bs->buffer[pos] = swap32(bs->buffer[pos]);
 
         if (pos + 1 == bs->max_size_in_dword) {
Sean V Kelley
2017-01-18 17:12:33 UTC
Permalink
Post by Sean V Kelley
Post by Sean V Kelley
shift uint32_t by 32 bits is undefined behavior.
For this particular case: when invoke avc_bitstream_put_ui() with 32
bits value at byte position of multiple of 4, existing 32 bits garbage
data in the buffer may be retained instead of cleared. The result is,
the position of NALU start code (0x00000001) looks like overwritten by
garbage value.
https://chromium-review.googlesource.com/#/c/410541/
---
src/i965_encoder_utils.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/i965_encoder_utils.c b/src/i965_encoder_utils.c
index ac58cd1a..e061d071 100644
--- a/src/i965_encoder_utils.c
+++ b/src/i965_encoder_utils.c
@@ -134,7 +134,11 @@ avc_bitstream_put_ui(avc_bitstream *bs, unsigned
int val, int size_in_bits)
Post by Sean V Kelley
bs->buffer[pos] = (bs->buffer[pos] << size_in_bits | val);
} else {
size_in_bits -= bit_left;
- bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >>
size_in_bits);
Post by Sean V Kelley
+ if (bit_left == 32) {
+ bs->buffer[pos] = (val >> size_in_bits);
The input value of size_in_bits should be less than or equal to 32, so now the
value of size_in_bits is 0 for this case, I think "bs->buffer[pos] = val"
is
more clear.
That's a reasonable change. Thanks Haihao. I'll update the patch.

Sean
Post by Sean V Kelley
Post by Sean V Kelley
+ } else {
+ bs->buffer[pos] = (bs->buffer[pos] << bit_left) | (val >>
size_in_bits);
Post by Sean V Kelley
+ }
bs->buffer[pos] = swap32(bs->buffer[pos]);
if (pos + 1 == bs->max_size_in_dword) {
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
--
Sean V. Kelley <***@intel.com>
Open Source Technology Center / SSG
Intel Corp.
Loading...