On 2012/04/19 19:49, Jurgen Kramer <gtmkra...@xs4all.nl> wrote:
> struct dsdiff_metadata {
> unsigned sample_rate, channels;
> + bool fileisdff;
> + uint32_t bitreverse;
This should be a bool, uint32_t is an odd choice.
> + uint64_t chunk_size;
> };
>
> static bool lsbitfirst;
>
> +struct dsdiff_header_dsf {
> + struct dsdiff_id id;
> + uint32_t size_low, size_high; /* DSD chunk size, including id
> = 28 */
> + uint32_t fsize_low, fsize_high; /* Total file size */
> + uint32_t pmeta_low, pmeta_high; /* Pointer to id3v2 metadata,
> should be at the end of the file */
Documentation like this is unreadable because it exceeds 80 columns.
Please use doxygen multi-line syntax like the rest of MPD.
> +};
> +
> +struct dsdiff_dsf_fmt_chunk { /* -- DSF file fmt
> chunk -- */
> + struct dsdiff_id id; /* "fmt " */
> + uint32_t size_low, size_high; /* fmt chunk size, including
> id, normally 52 */
> + uint32_t version; /* Version of this format = 1 */
> + uint32_t formatid; /* 0: DSD raw */
> + uint32_t channeltype; /* Channel Type, 1 = mono, 2 =
> stereo, 3 = 3 channels, etc */
> + uint32_t channelnum; /* Channel number, 1 = mono, 2
> = stereo, ... 6 = 6 channels */
> + uint16_t sfreq_low, sfreq_high; /* Sample frequency: 2822400,
> 5644800 */
Why split this well-aligned 32 bit integer into two 16 bit ints?
(I understand you splitted the 64 bit ints because they were not
properly aligned, which causes problems for some old CPUs like ARMv5)
> + uint32_t bitssample; /* Bits per sample 1 or 8 */
> + uint32_t scount_low, scount_high; /* Sample count per channel in
> bytes */
> + uint16_t blkszchl_low, blkszchl_high; /* Block size per channel =
> 4096 */
Again, why this split?
> @@ -274,28 +306,103 @@ dsdiff_read_metadata(struct decoder *decoder, struct
> input_stream *is,
> struct dsdiff_header header;
> if (!dsdiff_read(decoder, is, &header, sizeof(header)) ||
> !dsdiff_id_equals(&header.id, "FRM8") ||
> - !dsdiff_id_equals(&header.format, "DSD "))
> + !dsdiff_id_equals(&header.format, "DSD ")) {
> +
> + /* It's not a DFF file, check if it is a DSF file */
> + if (!dsdiff_skip_to(decoder, is, 0)) /* Reset to beginning
> of the stream */
> + return false;
Wrong indentation, code is unreadable.
> + if (chunk_size != 28)
What is this magic number? Can it be expressed with "sizeof"?
> + if ( fmt_chunk_size != 52 )
> + return false;
What is 52? (and wrong code style)
> - if (!dsdiff_skip_to(decoder, is, chunk_end_offset))
> + uint32_t samplefreq =
> ((uint32_t)GUINT16_FROM_LE(dsf_fmt_chunk.sfreq_high)) << 16 |
> + ((uint32_t)GUINT16_FROM_LE(dsf_fmt_chunk.sfreq_low));
Needlessly reassembling the splitted 32 bit int?
> + /* For now, only support version 1 of the standard, DSD raw stereo
> files with
> + a sample freq of 2822400 Hz */
> +
> + if (!(dsf_fmt_chunk.version == 1 && dsf_fmt_chunk.formatid == 0
>
> + && dsf_fmt_chunk.channeltype == 2
> + && dsf_fmt_chunk.channelnum == 2
> + && samplefreq == 2822400 ))
Wrong indent. (This !() looks rather obscure)
> + return false;
> +
> + uint32_t chblksize =
> ((uint32_t)GUINT16_FROM_LE(dsf_fmt_chunk.blkszchl_high)) << 16 |
> + ((uint32_t)GUINT16_FROM_LE(dsf_fmt_chunk.blkszchl_low));
> +
> + /* According to Sony spec block size should always be 4096 */
> + if ( chblksize != 4096 )
>
Many many stray tabs.
> + /* Record bits per sample format, needed to determine
> + if bitreverse is needed */
> + metadata->bitreverse = dsf_fmt_chunk.bitssample == 1 ? true : false;
The uint32_t gets assigned with a bool value (see above).
> + metadata->fileisdff = false;
> + return true;
> +
> + } else {
> +
> + /* Continue processing of a DFF format file */
> + while (true) {
> + if (!dsdiff_read_chunk_header(decoder, is,
> chunk_header))
> return false;
> +
Stray tab character.
> + if (dsdiff_id_equals(&chunk_header->id, "PROP")) {
> + if (!dsdiff_read_prop(decoder, is, metadata,
> + chunk_header))
> + return false;
> + } else if (dsdiff_id_equals(&chunk_header->id, "DSD "))
> {
> + /* done with metadata */
> + metadata->fileisdff = true; /* mark as DFF
> file */
More lines that are too long for no good reason.
> + return true;
> + } else {
> + /* ignore unknown chunk */
> +
> + uint64_t chunk_size =
> dsdiff_chunk_size(chunk_header);
> + goffset chunk_end_offset = is->offset +
> chunk_size;
> +
> + if (!dsdiff_skip_to(decoder, is,
> chunk_end_offset))
> + return false;
> + }
> }
> }
> }
> @@ -307,15 +414,45 @@ bit_reverse_buffer(uint8_t *p, uint8_t *end)
> *p = bit_reverse(*p);
> }
>
> +static void
> +make_dff_format(uint8_t *p, uint8_t *q, size_t nrbytes)
What does this function do?
> + if (!fileisdff)
> + make_dff_format(buffer, dsf_scratch_buffer, nbytes);
Use DFF code when it's _not_ DFF? This confuses me.
> /* success: file was recognized */
> -
> +
> decoder_initialized(decoder, &audio_format, false, -1);
Hunk adds a tab character.
> while (true) {
[...]
> + if (!metadata.fileisdff) {
This condition does not change in the loop. It would be better to
have it outside the loop, and have two loops. Two separate functions,
to avoid breaking the "80 column" rule, to save one indent level. The
code below looks a bit ugly due to excessive indentation.
> + if (!dsdiff_decode_chunk(decoder, is,
> + metadata.channels,
> + chunk_size,
> + metadata.fileisdff,
> + metadata.bitreverse))
> + break;
Wrong indent.
------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
