bugGNU Octave - Bugs: bug #47464, audioformats: a new function to...

 
 

bug #47464: audioformats: a new function to get information on supported audio formats

Submitted by:  Carnë Draug <carandraug>
Submitted on:  Fri 18 Mar 2016 11:48:22 PM UTC  
 
Category: Octave FunctionSeverity: 1 - Wish
Priority: 5 - NormalItem Group: Feature Request
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 16 May 2016 06:40:33 PM UTC, comment #10:

Ok, I have pushed your patch to the default branch here, with a proper commit message:

http://hg.savannah.gnu.org/hgweb/octave/rev/88657798b650

There were still a lot of style and organization things I fixed on top, please review the changes:

http://hg.savannah.gnu.org/hgweb/octave/rev/f7850bd66042

And further, new functions should be added to the NEWS file as well as to the user manual, see:

http://hg.savannah.gnu.org/hgweb/octave/rev/2e0e4eec0d62

Thanks for your contribution to Octave!

Mike Miller <mtmiller>
Project Administrator
Fri 13 May 2016 10:58:31 PM UTC, comment #9:

Here is the improvement on the remarks you made.

(file #37148)

Damjan Angelovski <damjangelovski>
Thu 12 May 2016 11:26:40 PM UTC, comment #8:

Damjan, are you interested in continuing to work on this patch?

Mike Miller <mtmiller>
Project Administrator
Tue 19 Apr 2016 11:08:25 PM UTC, comment #7:

The 3rd revision is much clearer and better.

Some minor coding style issues, please pay attention to whitespace, and have your editor delete trailing whitespace if that's an option, you have several blank lines that contain indented whitespace.

  • Consistently use space after function name and before opening parenthesis, but not when indexing an array.
  • Always use space around operators, e.g. "<", "<=", "=", "==", "!=", and "<<" in this patch.

The function should be named "audioformats". It also needs some actual usage documentation in its texinfo header.

This is a pretty good submission. If you'd like to try to work on some of these remaining issues, please do, but I can also take it as is and clean it up myself.

Mike Miller <mtmiller>
Project Administrator
Sat 09 Apr 2016 04:48:54 PM UTC, comment #6:

By "tidy up the code" I meant fixing most of the things you mentioned.
Should we put the underscore usage instead of CamelCase in http://wiki.octave.org/C%2B%2B_style_guide ?
This version:
- is C++ only
- can be passed an argument for searching formats (case insensitive)
- code is properly formatted (hopefully)

(file #36877)

Damjan Angelovski <damjangelovski>
Fri 08 Apr 2016 05:34:43 PM UTC, comment #5:

Agree with Mike on that. But the m file audioformats is pretty much a copy paste of the code in imformats. Even the tests are verbatim copies from imformats (and test imformats instead of the new audioformats function).

Carnë Draug <carandraug>
Project Member
Wed 30 Mar 2016 07:42:06 PM UTC, comment #4:

Very preliminary review, not yet applied/built/tested:

  • Style needs a lot of work, this is very important when working in Octave. Use appropriate spacing, use appropriate brace indentation, use underscores instead of CamelCase, don't comment out unused code.
  • I don't think there's any value to having audioformats defined in an m-file. You've shown with this latest patch that most of the work is easily done in C++, so why not just do it all in C++?

I'd say focus more on fixing these basic issues before worrying about things like tests, error checking, and passing a file format argument.

Mike Miller <mtmiller>
Project Administrator
Tue 29 Mar 2016 09:15:33 PM UTC, comment #3:

This implementation:
- queries the sndfile library with functions _NumAudFormats_ __AudMajorFormats__ and _AudSubFormats_ in audioread.cc
- has no documentation
- has no error handling
- has no formal tests
- the old audformats function is commented because we dont need add/remove functionality (and no error handling yet)
- the pretty_print is commented as we have a list of lists here and a table representation wont work well
- you cant pass a format as an argument, to get its specs

I will fix the things mentioned above (where needed) and will tidy up the code. Just need to know if it is good so far.

(file #36796)

Damjan Angelovski <damjangelovski>
Sun 20 Mar 2016 10:26:22 PM UTC, comment #2:

Thanks for offering to work on this feature. I think this is not quite the right direction for this new function. What we really want to do is query the sndfile C library for the audio formats that it supports rather than just filling in a hardcoded table of file formats.

The "add" and "remove" functions also make no sense for audioread and audiowrite because we don't have a way to install new audio format codecs.

Mike Miller <mtmiller>
Project Administrator
Sun 20 Mar 2016 08:08:54 PM UTC, comment #1:

Here is an initial implementation that is based on imformats. All the values are hardcoded, as there is no GraphicsMagick for audio and auinfo is not implemented. In this version only wav, flac and ogg formats are written as the only documentation i found on audio formats is https://www.gnu.org/software/octave/doc/interpreter/Audio-File-Utilities.html

(file #36702)

Damjan Angelovski <damjangelovski>
Fri 18 Mar 2016 11:48:22 PM UTC, original submission:

For image IO, octave has imformats which returns a struct array with information of all supported image formats. A similar function could exist for audio. This would make it simple to check for support of specific audio files.

This becomes more important now that checking for configuration values in octave_config_info has been "demoted".

Like imformats, it could be possible to use such audioformats function to control the audioread and audiowrite functions. In theory, a package could register with audioformats to add support for extra formats or even replace the existing backends for existing formats. In practice, that can be tricky and I don't anyone that uses imformats for that.

Still, a way to query support for specific audio formats would be useful.

Carnë Draug <carandraug>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #37148:  audioformats.diff added by damjangelovski (4KiB - text/x-patch)
file #36877:  audformats3.diff added by damjangelovski (3KiB - text/x-patch)
file #36796:  audformats2.diff added by damjangelovski (14KiB - text/x-patch - audformats with sndfile (v.2))
file #36702:  audformats.diff added by damjangelovski (12KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by damjangelovski (Updated the item)
  • -unavailable- added by carandraug (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 16 May 2016 06:40:33 PM UTCmtmillerStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Fri 13 May 2016 10:58:31 PM UTCdamjangelovskiAttached File-=>Added audioformats.diff, #37148
    Sat 09 Apr 2016 04:48:54 PM UTCdamjangelovskiAttached File-=>Added audformats3.diff, #36877
    Wed 30 Mar 2016 07:42:06 PM UTCmtmillerStatusNone=>In Progress
    Tue 29 Mar 2016 09:15:33 PM UTCdamjangelovskiAttached File-=>Added audformats2.diff, #36796
    Sun 20 Mar 2016 08:08:54 PM UTCdamjangelovskiAttached File-=>Added audformats.diff, #36702

    Back to the top


    Powered by Savane 3.1-cleanup1