#179 Zero padding added by --sector-align is not zero

closed-fixed
Josh Coalson
None
4
2014-08-24
2005-07-13
Dave Chapman
No

When using the --sector-align option with flac (tested
with 1.1.1 and 1.1.2 on x86 Linux, but I have read a
report of Windows users experiencing the same problem)
the buffer used to zero-pad the last file to a sector
boundary is not fully initialised to zero.

The result is that there is non-zero data appended to
the end of the last WAV file in the set being
sector-aligned.

This was identified by doing MD5 comparisons of files
sector aligned using the "shntool" utility and the same
files sector-aligned using flac.

My test consisted of the creation of four "non-aligned"
CD quality WAV files. I then "fixed" these files using
both the "shntool fix" command and the "flac
--sector-align" command.

I then compared the WAV files created by shntool and
the WAV files
created by uncompressing the FLAC files, and the MD5
checksums did not match for the final track. They did
match for the first three tracks.

On comparing the contents of the WAV files, the
"shntool" fixed version correctly contained 448 zero
bytes at the end (112 samples). However, the FLAC
version contained 224 zero bytes, followed by 224
non-zero bytes.

I've think I've tracked the bug to the calculation of
the number of bytes to fill the input_ buffer with,
which is calculated in line 778 of encode.c (v1.1.2 and
current CVS) as follows:

data_bytes = wide_samples * (bps >> 3);

However, my understanding of the code is that the
input_buffer is always an array of 32-bit integers, and
therefore the calculation should always be:

data_bytes = wide_samples * 4;

(or a macro that evaluates to 4, whatever you think is
appropriate).

The expression (bps >> 3) is also used elsewhere in
encode.c so it is possible that other, similar bugs
exist - but I have not looked for them.

Discussion

  • Logged In: NO

    it incorrectly assumes that input_ bps equals source bps,
    however source bps is 16 (in this case!) while input_ bps is
    32 (always)

    Dave, (bps >> 3) is equivalent to (bps / 8) and thus
    converts input file's bits per sample to input file's bytes
    per sample (btw a "sample" contains one sample from one
    channel, while a "wide sample" contains one sample from all
    channels)

     
  • Josh Coalson
    Josh Coalson
    2005-08-09

    • assigned_to: nobody --> jcoalson
    • status: open --> open-accepted
     
  • Josh Coalson
    Josh Coalson
    2005-08-27

    • status: open-accepted --> open-fixed
     
  • Josh Coalson
    Josh Coalson
    2005-08-27

    Logged In: YES
    user_id=78173

    ok, fixed in CVS, thanks.

     
  • Josh Coalson
    Josh Coalson
    2005-09-03

    • priority: 5 --> 4
     
  • Josh Coalson
    Josh Coalson
    2006-06-14

    • status: open-fixed --> closed-fixed