On 29/03/2024 15:58, Andreas Rheinhardt wrote:
Mark Thompson:
On 29/03/2024 14:00, Andreas Rheinhardt wrote:
James Almer:
On 3/29/2024 10:10 AM, Mark Thompson wrote:
On 28/03/2024 13:15, tong1.wu-at-intel....@ffmpeg.org wrote:
From: Tong Wu <tong1...@intel.com>
HEVCHdrParams* receives a pointer which points to a dynamically
allocated memory block. It causes the memcmp always returning 1.
Add a function to do the comparision. A condition is also added to
avoid malloc(0).
Signed-off-by: Tong Wu <tong1...@intel.com>
---
libavcodec/hevc_ps.c | 20 ++++++++++++++++----
libavcodec/hevc_ps.h | 4 +++-
2 files changed, 19 insertions(+), 5 deletions(-)
It doesn't seem like this method works at all, even before the recent
change with the pointer.
Structs can contain arbitrary padding, and any write to the struct
makes the padding unspecified. memcmp() is therefore never valid as a
method of comparing after writing some fields, as done here. (It
could only be valid if the structs compared were made by memcpy() with
no fields written directly.)
The struct is zero allocated, so shouldn't the padding be exactly the
same for two equal VPSs after parsing?
In practice it is (and the current code already relied on this); yet as
has already been said padding bytes take unspecified values at any store
(to any member). In practice, if the compiler uses instructions that
clobber the padding, the padding in both structs is clobbered in the
same way.
This seems like a strong assumption beyond that of the C specification
which needs to be documented.
Are you expecting that there is no case where ABI-undefined top bits of
registers can leak into the padding fields, or that all functions called
here will necessarily set those top bits to the same values, or
something else?
Yes, I am expecting that. That is also what the code already relied on
before the addition of the allocated field and there have been no
reports that this caused issues.
Huh. Having just experimented a bit I find myself surprised by the lengths
x86-64 gcc goes to with weird unaligned accesses to avoid this (e.g. to write
to aligned uint8_t a[31] it may do aligned 16-byte write to a and unaligned
16-byte write to a+15, avoiding touching the padding for no clear reason).
Are you aware of any documentation from gcc or llvm about on what they
guarantee here?
This does not change that I consider it crazy to remove the parameter
sets referencing a parameter set that is removed.
I agree, having now read the code more. My comment was purely driven by
observing the use of memcmp() on structs (an operation well-known to be very
difficult to use in standard C), not by looking carefully at the rest of the
code involved.
- Mark
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
