Skip to content

Conversation

@pkrcel
Copy link
Contributor

@pkrcel pkrcel commented Jan 2, 2015

Pull request for feature in the title.

I've talked about this in this allegro.cc thead: .VOC (Creative Voice File) samples playback with Allegro.

Actual decoding is done through libsndfile for which I added support in the build system.

Comments are appreciated, I'll be anyway sending a patch to the mailing list for proper procedure.

Discussion Topics:

  • libsndfile can load a moltitude of formats, might consider a wider support?
  • Licensing: libsndfile lives under the GNU LGPL v2.1 or v3 license link, this means that on systems on which allegro will not allow dynamic linking of the decoding lib (all but Windows?) it would force the GNU GPL on the whole work.
  • Allegro 4 already supported Creative Voice...have I done wrong in not porting the decoder itself? (I couldn't find a guideline about this)

@pkrcel
Copy link
Contributor Author

pkrcel commented Jan 5, 2015

As per Beoran's observations in the linked thread, I've pushed a different implementation moving out libsndfile to a DIFFERENT file (which can be included or not) and created (I wouldn't say ported from A4) a new internal decoder for VOC files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this all about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MSYS2 environment, I had cmake which did not find gdiplus. Adding a suggested name libgdiplus fixed the problem

@SiegeLord
Copy link
Member

Is there a (very small) .voc file we could add to the examples/data to make sure there's something to test this on?

@pkrcel
Copy link
Contributor Author

pkrcel commented Jan 14, 2015

They are extremely small, but the ones I am working on are (most probably) covered by copyrights, since I am working with Master of Orion 2 samples.

I GUESS I could search for some free samples or conver the ones we have with SoX.

I'll look after it when I'm back from off-site duties (give me a couple days or whenabouts).

@SiegeLord
Copy link
Member

I looked a bit more closely at this. I think it looks pretty good, I can fix the rest of the minor style issues myself. One thing that you should fix is that you never check the return value of al_fread, there's no guarantee that it will actually read the amount of bytes you request.

@pkrcel
Copy link
Contributor Author

pkrcel commented Jan 16, 2015

Will check the al_read functions and add return value control when applicable.

But could you pls let me know what are the style problems still present? That way I won't need to look at the diff post-work and I'll be able to contribute more properly to allegro next time.

Cheers
-pkrcel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra semicolon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space before the 2.

@pkrcel
Copy link
Contributor Author

pkrcel commented Jan 17, 2015

Hope to have got all of them, I looked into the while file several times, but it's just like when you try to proofread your own essay for the billionth time....my eyes just keep crossing ^__^"

@SiegeLord
Copy link
Member

Merged as 00a42a3 and 1d85b70. Thanks a lot!

@SiegeLord SiegeLord closed this Jan 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants