Skip to content

Commit

Permalink
Revert "util/bitstream.cpp: Fixed cases where bits would be dropped w…
Browse files Browse the repository at this point in the history
…hen reading and writing. (#12057)"

This reverts commit 69c3cd7.

This causes CHD SHA1 digests to change.  Either it's buggy, or CHD SHA1
digests depend on the representation rather than the data itself.
  • Loading branch information
cuavas committed Feb 23, 2024
1 parent 69c3cd7 commit 229d19d
Showing 1 changed file with 13 additions and 45 deletions.
58 changes: 13 additions & 45 deletions src/lib/util/bitstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ class bitstream_in
private:
// internal state
uint32_t m_buffer; // current bit accumulator
int m_bits; // number of bits in the accumulator
int m_bits; // number of bits in the accumulator
const uint8_t * m_read; // read pointer
uint32_t m_doffset; // byte offset within the data
uint32_t m_dlength; // length of the data
int m_dbitoffs; // bit offset within current read pointer
};


Expand All @@ -65,7 +64,7 @@ class bitstream_out
private:
// internal state
uint32_t m_buffer; // current bit accumulator
int m_bits; // number of bits in the accumulator
int m_bits; // number of bits in the accumulator
uint8_t * m_write; // write pointer
uint32_t m_doffset; // byte offset within the data
uint32_t m_dlength; // length of the data
Expand All @@ -86,8 +85,7 @@ inline bitstream_in::bitstream_in(const void *src, uint32_t srclength)
m_bits(0),
m_read(reinterpret_cast<const uint8_t *>(src)),
m_doffset(0),
m_dlength(srclength),
m_dbitoffs(0)
m_dlength(srclength)
{
}

Expand All @@ -105,31 +103,12 @@ inline uint32_t bitstream_in::peek(int numbits)
// fetch data if we need more
if (numbits > m_bits)
{
while (m_bits < 32)
while (m_bits <= 24)
{
uint32_t newbits = 0;

if (m_doffset < m_dlength)
{
// adjust current data to discard any previously read partial bits
newbits = (m_read[m_doffset] << m_dbitoffs) & 0xff;
}

if (m_bits + 8 > 32)
{
// take only what can be used to fill out the rest of the buffer
m_dbitoffs = 32 - m_bits;
newbits >>= 8 - m_dbitoffs;
m_buffer |= newbits;
m_bits += m_dbitoffs;
}
else
{
m_buffer |= newbits << (24 - m_bits);
m_bits += 8 - m_dbitoffs;
m_dbitoffs = 0;
m_doffset++;
}
m_buffer |= m_read[m_doffset] << (24 - m_bits);
m_doffset++;
m_bits += 8;
}
}

Expand Down Expand Up @@ -217,11 +196,8 @@ inline bitstream_out::bitstream_out(void *dest, uint32_t destlength)

inline void bitstream_out::write(uint32_t newbits, int numbits)
{
newbits <<= 32 - numbits;

// flush the buffer if we're going to overflow it
while (m_bits + numbits >= 32 && numbits > 0)
{
if (m_bits + numbits > 32)
while (m_bits >= 8)
{
if (m_doffset < m_dlength)
Expand All @@ -231,19 +207,11 @@ inline void bitstream_out::write(uint32_t newbits, int numbits)
m_bits -= 8;
}

// offload more bits if it'll still overflow the buffer
if (m_bits + numbits >= 32)
{
const int rem = std::min(32 - m_bits, numbits);
m_buffer |= newbits >> m_bits;
m_bits += rem;
newbits <<= rem;
numbits -= rem;
}
}

if (numbits <= 0)
return;
// shift the bits to the top
if (numbits == 0)
newbits = 0;
else
newbits <<= 32 - numbits;

// now shift it down to account for the number of bits we already have and OR them in
m_buffer |= newbits >> m_bits;
Expand Down

1 comment on commit 229d19d

@987123879113
Copy link
Contributor

@987123879113 987123879113 commented on 229d19d Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I was doing some more thorough checks I had noticed the SHA1 value was different when I extracted and recreated CHDs so I requested a revert just to be sure due to the release deadline, but after more checks I don't think this PR had anything to do with it.
I was able to track down a change to an older PR of mine (#10231) that fixed an actual bug which resulted in data being changed, but even older than that (I checked up to 0.228 but it's even older) there was another change that changed the PGSUB metadata field's value that seems to be causing the overall SHA1 to change.

More recently created CHDs seem to be recreateable with the same SHA1s.

Example of old (2013) CHD (PGSUB:RW):

chdman - MAME Compressed Hunks of Data (CHD) manager 0.262 (mame0252-3659-gcd48f6054c4)
Input file:   roms/dstage/845aaa02.chd
File Version: 5
Logical size: 339,194,880 bytes
Hunk Size:    19,584 bytes
Total Hunks:  17,320
Unit Size:    2,448 bytes
Total Units:  138,560
Compression:  cdlz (CD LZMA), cdzl (CD Deflate), cdfl (CD FLAC)
CHD size:     215,422,965 bytes
Ratio:        63.5%
SHA1:         9b786de9b1085009c088de0d40425976c1f8df7b
Data SHA1:    7f4f3c579ad39cedaa163538ef1cf431babd8ce7
Metadata:     Tag='CHT2'  Index=0  Length=91 bytes
              TRACK:1 TYPE:MODE1_RAW SUBTYPE:RW_RAW FRAMES:8687 PREGAP:0 P

chdman - MAME Compressed Hunks of Data (CHD) manager 0.262 (mame0252-3659-gcd48f6054c4)
TRACK:1 TYPE:MODE1_RAW SUBTYPE:RW_RAW FRAMES:8687 PREGAP:0 PGTYPE:MODE1 PGSUB:RW POSTGAP:0

Recreated (PGSUB:NONE):

chdman - MAME Compressed Hunks of Data (CHD) manager 0.262 (mame0252-3659-gcd48f6054c4)
Input file:   /Users/owner/chd_test/chds/test5.chd
File Version: 5
Logical size: 339,194,880 bytes
Hunk Size:    19,584 bytes
Total Hunks:  17,320
Unit Size:    2,448 bytes
Total Units:  138,560
Compression:  cdlz (CD LZMA), cdzl (CD Deflate), cdfl (CD FLAC)
CHD size:     215,131,093 bytes
Ratio:        63.4%
SHA1:         cfcc97030de424b0f33cdbac857987e425681011
Data SHA1:    7f4f3c579ad39cedaa163538ef1cf431babd8ce7
Metadata:     Tag='CHT2'  Index=0  Length=93 bytes
              TRACK:1 TYPE:MODE1_RAW SUBTYPE:RW_RAW FRAMES:8687 PREGAP:0 P

chdman - MAME Compressed Hunks of Data (CHD) manager 0.262 (mame0252-3659-gcd48f6054c4)
TRACK:1 TYPE:MODE1_RAW SUBTYPE:RW_RAW FRAMES:8687 PREGAP:0 PGTYPE:MODE1 PGSUB:NONE POSTGAP:0

Please sign in to comment.