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

 
 

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

Submitter:  Carnë Draug <carandraug>
Submitted:  Fri 18 Mar 2016 11:48:22 PM UTC
   
 
Category:  Octave Function Severity:  1 - Wish
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group Member
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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group Member
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>
Group Member

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by damjangelovski (Updated the item)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-16 mtmiller StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-05-13 damjangelovski Attached File- Added audioformats.diff, #37148
    2016-04-09 damjangelovski Attached File- Added audformats3.diff, #36877
    2016-03-30 mtmiller StatusNone In Progress
    2016-03-29 damjangelovski Attached File- Added audformats2.diff, #36796
    2016-03-20 damjangelovski Attached File- Added audformats.diff, #36702

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code