New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] MP3 support #322

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@foolswood
Contributor

foolswood commented Sep 20, 2017

Putting this up early (in a distinctly unpolished state) to get some feedback about the overall shape and approach.

Status summary:

  • Read support
  • Write support
  • Make optional in build
  • dlopen lame

@foolswood foolswood referenced this pull request Sep 20, 2017

Open

Add MP3 support #258

@erikd

This comment has been minimized.

Show comment
Hide comment
@erikd

erikd Sep 26, 2017

Owner

however in order to test it an example file to load is required. Is this acceptable, and if so where should this be in the source tree?

libsndfile has been around for nearly 20 years and has never had a binary file committed to the repository. i would like to keep it that way :). Hence, basics of read and write should be added in one go.

Owner

erikd commented Sep 26, 2017

however in order to test it an example file to load is required. Is this acceptable, and if so where should this be in the source tree?

libsndfile has been around for nearly 20 years and has never had a binary file committed to the repository. i would like to keep it that way :). Hence, basics of read and write should be added in one go.

@foolswood

This comment has been minimized.

Show comment
Hide comment
@foolswood

foolswood Sep 26, 2017

Contributor

Fair enough, I'll have a stab at that. Did you have any comments on what's there so far?

Contributor

foolswood commented Sep 26, 2017

Fair enough, I'll have a stab at that. Did you have any comments on what's there so far?

@foolswood foolswood changed the title from [WIP] MP3 read support to [WIP] MP3 support Sep 26, 2017

if (lame_handle.init == NULL)
{ failed = 1 ;
break ;
}

This comment has been minimized.

@erikd

erikd Sep 28, 2017

Owner

Those braces don't match the coding style of the rest of the libsndfile code base,

The Scripts/cstyle.py script catches some, but not all issue.

@erikd

erikd Sep 28, 2017

Owner

Those braces don't match the coding style of the rest of the libsndfile code base,

The Scripts/cstyle.py script catches some, but not all issue.

This comment has been minimized.

@erikd

erikd Oct 1, 2017

Owner

I've just added some new rules to the Script/cstyle.py script that should help with some more coding style issues. The one that's difficult to add to that script is the closing curly indentation which the current source code uses. It should be like:

    if (whatever)
    {   do_soometing ;
        } ;

Notice how the closing curly is indented one more level than the open curly brace.

@erikd

erikd Oct 1, 2017

Owner

I've just added some new rules to the Script/cstyle.py script that should help with some more coding style issues. The one that's difficult to add to that script is the closing curly indentation which the current source code uses. It should be like:

    if (whatever)
    {   do_soometing ;
        } ;

Notice how the closing curly is indented one more level than the open curly brace.

return mp3_write_open (psf) ;
default:
return SFE_UNIMPLEMENTED ;
}

This comment has been minimized.

@erikd

erikd Sep 28, 2017

Owner

The libsndfile codebase has an coding style which it adheres to with a reasonably high level of consistency.

The is an a Python script Scripts/cstyle.py which catchs a large number of issues. You should still have a look at the other code and match the style.

@erikd

erikd Sep 28, 2017

Owner

The libsndfile codebase has an coding style which it adheres to with a reasonably high level of consistency.

The is an a Python script Scripts/cstyle.py which catchs a large number of issues. You should still have a look at the other code and match the style.

This comment has been minimized.

@foolswood

foolswood Sep 28, 2017

Contributor

I thought I had got it all past cstyle.py (which I believe is triggered in the pre-commit hook), but I'll check elsewhere for the preferred bracket style.

@foolswood

foolswood Sep 28, 2017

Contributor

I thought I had got it all past cstyle.py (which I believe is triggered in the pre-commit hook), but I'll check elsewhere for the preferred bracket style.

int decoder_err = MPG123_OK ;
mpg123_handle * decoder ;
if (mpg123_initialised == 0)
{ int decoder_init_err = mpg123_init () ;

This comment has been minimized.

@erikd

erikd Sep 28, 2017

Owner

Can mpg123_init() not be called multiple times without blowing up?

@erikd

erikd Sep 28, 2017

Owner

Can mpg123_init() not be called multiple times without blowing up?

mp3_read_close (SF_PRIVATE * psf)
{ mpg123_handle * decoder = psf->codec_data ;
if (decoder != NULL)
{ // mpg123_close(decoder); <- Not sure if we need this?

This comment has been minimized.

@erikd

erikd Sep 28, 2017

Owner

If it doesn't blow up, call it anyway.

@erikd

erikd Sep 28, 2017

Owner

If it doesn't blow up, call it anyway.

This comment has been minimized.

@foolswood

foolswood Sep 28, 2017

Contributor

IIRC it did, which is why it was commented out, but I'll check.

@foolswood

foolswood Sep 28, 2017

Contributor

IIRC it did, which is why it was commented out, but I'll check.

psf->codec_data = NULL ;
}
if (!--mpg123_initialised)
mpg123_exit () ;

This comment has been minimized.

@erikd

erikd Sep 28, 2017

Owner

This is kinda weird! What happens without that mpg123_exit() call? Does it leak memory?

@erikd

erikd Sep 28, 2017

Owner

This is kinda weird! What happens without that mpg123_exit() call? Does it leak memory?

This comment has been minimized.

@foolswood

foolswood Sep 28, 2017

Contributor

The mpg123_init() and mpg123_exit() calls are following the canonical documented use of the library. I've looked through the source, which suggests there is some cheating to be had and contacted the mpg123 developers to see what their future plans for the methods are.

@foolswood

foolswood Sep 28, 2017

Contributor

The mpg123_init() and mpg123_exit() calls are following the canonical documented use of the library. I've looked through the source, which suggests there is some cheating to be had and contacted the mpg123 developers to see what their future plans for the methods are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment