Opened 13 years ago

Closed 13 years ago

#2957 closed task (fixed)

Port to FLAC 1.1.3

Reported by: ankoe Owned by: Jarod Wilson
Priority: minor Milestone: unknown
Component: mythmusic Version: head
Severity: low Keywords: flac 1.1.3
Cc: Ticket locked: no

Description

I think it makes sense to upgrade to flac 1.1.3, it has nice enhancements (details here) But version 1.1.3 is not compatible to 1.1.2, there is a porting guide

Attachments (2)

flacfix_r12810.patch (4.8 KB) - added by mythtv@… 13 years ago.
Compilation fixes for commit 12810 and FLAC 1.1.2
mythmusic-flac113support-cleanups.patch (14.0 KB) - added by Jarod Wilson 13 years ago.
Patch to greatly reduce duplication and size of define blocks

Download all attachments as: .zip

Change History (18)

comment:1 Changed 13 years ago by VMiklos <vmiklos@…>

comment:2 Changed 13 years ago by Jarod Wilson

A good start, but this patch will hose over anyone using flac 1.0.4 to 1.1.2. I believe we'll need to add some #ifdef'ing to prevent breakage, which the flac folks were kind enough to give an example of here:

http://flac.sourceforge.net/api/group__porting.html

I'll see if I can't make that happen...

comment:3 Changed 13 years ago by Stuart Auchterlonie

Owner: changed from Isaac Richards to Jarod Wilson

comment:4 Changed 13 years ago by Jarod Wilson

Okay, I've got a patch all #ifdef'd to hell and back that builds cleanly against flac 1.1.4 now, and *should* build against 1.1.2 as well (I'll try that shortly). Also need to see if flac encoding and decoding still works in both cases...

http://wilsonet.com/jarod/mythmusic-0.20-flac113.patch

comment:5 Changed 13 years ago by Jarod Wilson

Does indeed build against 1.1.2 as well, and test encodings pass basic sanity check. Committing shortly...

comment:6 Changed 13 years ago by Jarod Wilson

Resolution: fixed
Status: newclosed

(In [12810]) Add support for new flac API introduced in 1.1.3. Rather insane to have a huge API change from 1.1.2 to 1.1.3, but whatever... At least the flac folks wrote some decent docs on porting to the new API... Builds cleanly against both flac 1.1.2 and 1.1.4, and basic sanity checks show it appears to be doing the right thing. Closes #2957.

comment:7 Changed 13 years ago by Nigel

Jarod, tried to get in before you committed, but failed :-( Was wondering if you could further simplify the #defines up the top by removing the duplication - using something like:

#if !defined(FLAC_API_VERSION_CURRENT) || FLAC_API_VERSION_CURRENT <= 7
  /* FLAC 1.0.4 to 1.1.2 */
  #include <FLAC/file_encoder.h>
  #define FLACENC FLAC__file_encoder
#else 
  /* FLAC 1.1.3 and up */
  #define NEWFLAC
  #include <FLAC/stream_encoder.h>
  #define FLACENC FLAC__stream_encoder
#endif
#define encoder_new() FLACENC_new()
#define encoder_setup(enc, streamable_subset, do_mid_side_stereo, \
            loose_mid_side_stereo, channels, bits_per_sample, \
            sample_rate, blocksize, max_lpc_order, \
            qlp_coeff_precision, do_qlp_coeff_prec_search, \
            do_escape_coding, do_exhaustive_model_search, \
            min_residual_partition_order, max_residual_partition_order, \
            rice_parameter_search_dist) \
            { \
              FLACENC_set_streamable_subset(enc, streamable_subset); \
...

et c. (same with FLACSeekable vs FLAC).

comment:8 Changed 13 years ago by Jarod Wilson

I thought about doing that, and then I forgot I'd thought about doing that... I'll take a crack at cleaning it up some tomorrow, its already past my bedtime. :)

Changed 13 years ago by mythtv@…

Attachment: flacfix_r12810.patch added

Compilation fixes for commit 12810 and FLAC 1.1.2

comment:9 Changed 13 years ago by mythtv@…

Resolution: fixed
Status: closedreopened

Commit 12810 doesn't compile for me. I'm using FLAC 1.1.2 and the problem is down to the lack of ';' at the end of the lines in the encoder_setup macro for FLAC versions 1.1.2 and below (see attached flacfix_r12810.patch). The patch also removes the '/' from the macro calls in the CPP files, this wasn't necessary to make it compile but the '/' aren't necessary to make it compile either.

comment:10 Changed 13 years ago by Stuart Auchterlonie

(In [12814]) Refs #2957. Fixes compilation after [12810] with flac 1.1.2

comment:11 Changed 13 years ago by Jarod Wilson

Gah. Now how the hell did that happen? I hit that same thing building against 1.1.2 and fixed it, but that must not have made it back over to my svn tree. Sorry 'bout that...

Didn't have time to poke at the duplication cleanups today, definitely should tomorrow though...

comment:12 Changed 13 years ago by Jarod Wilson

Oh yeah... The back-slashes in the cpp files make vim syntax highlighting much happier, but either or. :)

Changed 13 years ago by Jarod Wilson

Patch to greatly reduce duplication and size of define blocks

comment:13 Changed 13 years ago by Jarod Wilson

The above patch hasn't actually been tested yet, but it illustrates the approach I'm taking to reduce the duplication of defines. Will compile test shortly...

comment:14 Changed 13 years ago by Jarod Wilson

Hrm. So the approach nigel suggested (which is what I implemented) isn't working for me. Had someone else look at it, who thought it should be working, but now believes it simply doesn't work (tried something similar w/a very simple test program across multiple versions of gcc). Here's an example of what I'm seeing:

Header define in flacdecoder.h:
#define StreamDecoder FLAC__SeekableStreamDecoder

Line of code in flacdecoder.cpp:
static StreamDecoderReadStatus flacread(blah)

Error when compiling:
flacdecoder.cpp:20: error: 'StreamDecoderReadStatus' does not name a type

I'm somewhat inclined to leave well-enough alone at this point, unless there's an easy/obvious way to do this that I'm just missing...

comment:15 Changed 13 years ago by Nigel

Sorry. Yes, there is a trick to get GCC's CPP to join symbols - the ##:

#define FLACENC(a) FLAC__file_encoder ## a
#define encoder_new() FLACENC(_new)
#define encoder_setup(enc, streamable_subset) \
        FLACENC(_set_streamable_subset)(enc, BLAH, streamable_subset)

encoder_new();
encoder_setup(a,b);

But feel free to leave it as-is, because this trick probably requires GCC.

comment:16 Changed 13 years ago by Jarod Wilson

Resolution: fixed
Status: reopenedclosed

Ah, I'd played around with ## a little bit, but probably didn't quite get it right. Makes sense how that would work now. But yeah, if its going to require trickery that may not work universally, let's just leave it as-is.

Note: See TracTickets for help on using tickets.