Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
On Mon, Apr 23, 2012 at 07:16:49PM +0200, Jurgen Kramer wrote: Please show me how I can check myself. It is getting a bit tiresome to hear about wrong tabs/indents. Jurgen If you use Vim, this little snippet will highlight all trailing white space: au Syntax * \ syn match ExtraSpace \s\+$ containedin=ALL \ | hi def link ExtraSpace Error syn match ExtraSpace \s\+$ hi def link ExtraSpace Error Otherwise, a 'git grep \s$' should get you the respective lines. As for other white space errors, I don't know. Thanks, Jonathan Neuschäfer -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
At 2012-04-25 00:03:00 +0200, Jonathan Neuschäfer wrote: On Mon, Apr 23, 2012 at 07:16:49PM +0200, Jurgen Kramer wrote: Please show me how I can check myself. It is getting a bit tiresome to hear about wrong tabs/indents. Jurgen If you use Vim, this little snippet will highlight all trailing white space: au Syntax * \ syn match ExtraSpace \s\+$ containedin=ALL \ | hi def link ExtraSpace Error syn match ExtraSpace \s\+$ hi def link ExtraSpace Error Alternatively, switch on list mode, since this is the sort of thing it's meant for: :se list Also see the docs for the listchars option if you want to customize what is displayed. :he 'listchars' I have this in my vimrc: set list if encoding =~ utf-8 set listchars=tab:›\ ,trail:·,extends:…,precedes:…,nbsp:‸ else set listchars=tab:\ ,trail:.,extends:,precedes:,nbsp:. endif This also makes it very obvious when there are mixtures of spaces and tabs and things like that. --bart -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
On Mon, 2012-04-23 at 19:09 +0200, Max Kellermann wrote: On 2012/04/21 13:29, Jurgen Kramer gtmkra...@xs4all.nl wrote: - Adhered to max 80 colom width This is not doxygen syntax, and is barely readable. Please use doxygen syntax for documenting struct attributes. - Fixed indents (hopefully correctly) What did you fix? Indentation dsdiff_read_metadata() is still all wrong. I took a stab a changing the loop part. Please check, it probably needs some more work. I don't see what you changed. There are 17 whitespace errors in your patch according to git. Please show me how I can check myself. It is getting a bit tiresome to hear about wrong tabs/indents. Jurgen -- 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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
On 2012/04/21 13:29, Jurgen Kramer gtmkra...@xs4all.nl wrote: - Adhered to max 80 colom width This is not doxygen syntax, and is barely readable. Please use doxygen syntax for documenting struct attributes. - Fixed indents (hopefully correctly) What did you fix? Indentation dsdiff_read_metadata() is still all wrong. I took a stab a changing the loop part. Please check, it probably needs some more work. I don't see what you changed. There are 17 whitespace errors in your patch according to git. -- 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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
On 2012/04/21 19:21, Jonathan Neuschäfer j.neuschae...@gmx.net wrote:
On Sat, Apr 21, 2012 at 01:29:37PM +0200, Jurgen Kramer wrote:
+struct dsdiff_header_dsf {
+ struct dsdiff_id id;/* DSD */
+ 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 */
+};
What's the reason for not using uint64_t?
Because it might be misaligned, due to sizeof(dsdiff_id)==4, and some
CPUs may require 64 bit reads and 64 bit alignment, or MPD gets killed
with SIGBUS.
But since this struct is not explicitly packed (something which is
only possible with compiler extensions), the compiler would choose to
add a hole of 4 bytes after the dsdiff_id.
+ /* Check bits per sample format, determine if bitreverse is needed */
+ metadata-bitreverse = dsf_fmt_chunk.bitssample == 1 ? true : false;
@Max: Would it make sense to directly assign the value, which the '=='
operator returned?
Correct. Less code and easier to read.
Max
--
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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v3
On Sat, Apr 21, 2012 at 01:29:37PM +0200, Jurgen Kramer wrote:
+struct dsdiff_header_dsf {
+ struct dsdiff_id id;/* DSD */
+ 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 */
+};
What's the reason for not using uint64_t?
+ /* Check bits per sample format, determine if bitreverse is needed */
+ metadata-bitreverse = dsf_fmt_chunk.bitssample == 1 ? true : false;
@Max: Would it make sense to directly assign the value, which the '=='
operator returned?
+ metadata-fileisdff = false;
+ return true;
+
+ } else {
The indentation is a bit confusing here.
/**
+ * DSF data is build up of alternating 4096 blocks of DSD samples for left
and
+ * right data. Convert the buffer holding 1 block of 4096 DSD left samples
and
+ * 1 block of 4096 DSD right samples to 8k of samples in normal PCM
left/right
+ * order.
+ */
+static void
+dsf_to_pcm_order(uint8_t *p, uint8_t *q, size_t nrbytes)
You can see by reading the code that the content of 'p' is copied into
'q', but more descriptive names (like 'from' and 'to') would make it
more readable.
Thanks,
Jonathan Neuschäfer
--
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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder - v2
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;
+
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder
On Thu, 2012-04-19 at 17:49 +0200, Max Kellermann wrote: On 2012/04/19 17:41, Jurgen Kramer gtmkra...@xs4all.nl wrote: I do not mind putting DSF file format support in a separate decoder plugin. Having two separate decoders would probably yield better readable and therefore better maintainable code. On the other hand it would mean a bit of duplicated code and effort. Both file formats contain DSD data and output 'PCM'. Duplicate code is worse than that, so either leave it as a combined plugin, or move common code into a library used by both. The latter looks like the cleanest solution to me. Alright, can you live with a 2 step approach? 1. Start with adding DSF to current DSDIFF decoder 2. Create common library for DSF and DSDIFF and update DSDIFF decoder to use it. This way people can start enjoying playing DSF files soon(er). Jurgen -- 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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder
On 2012/04/19 17:50, Jurgen Kramer gtmkra...@xs4all.nl wrote: Alright, can you live with a 2 step approach? 1. Start with adding DSF to current DSDIFF decoder 2. Create common library for DSF and DSDIFF and update DSDIFF decoder to use it. Yes. -- 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
Re: [Musicpd-dev-team] [PATCH] Add support for DSF files to DSDIFF decoder
On 2012/04/14 17:05, Jurgen Kramer gtmkra...@xs4all.nl wrote: Attached patch adds support for DSF files to the DSDIFF decoder. It can be used in conjunction with DSD2PCM and DSD-over-USB. This looks like this supports a different file format and should live in another decoder plugin. Is there an advantage in implementing both formats in one plugin? I get build errors: src/decoder/dsdiff_decoder_plugin.c: In function 'dsdiff_read_metadata': src/decoder/dsdiff_decoder_plugin.c:376:2: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'goffset' [-Werror=format] src/decoder/dsdiff_decoder_plugin.c:357:11: error: unused variable 'samplecnt' [-Werror=unused-variable] src/decoder/dsdiff_decoder_plugin.c:330:11: error: unused variable 'metadata_offset' [-Werror=unused-variable] src/decoder/dsdiff_decoder_plugin.c:327:11: error: unused variable 'file_size' [-Werror=unused-variable] -- Better than sec? Nothing is better than sec when it comes to monitoring Big Data applications. Try Boundary one-second resolution app monitoring today. Free. http://p.sf.net/sfu/Boundary-dev2dev ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
