CDDA merge (0/x): overall review and merge plan

[kcgen]

... add support for MP3, FLAC, and Opus; add handling of mono and non-44.1KHz tracks; and improve internal handling of CD audio streams.

Notable changes (perhaps worthy of separate reviews):

• added build script
• grouped single CI build into operating system workflows
• used build script in CI builds for wider compiler and release type coverage
• improved DOSBox's internal CDROM image audio handling
• replaced dependence on SDL_Sound with internal handling
• replaced dependence on libvorbis with built-in stb_vorbis codec
• replaced dependence on libFLAC (which never worked) with built-in dr_flac codec
• replaced dependence on libMPEG123 (which never worked) with build-in dr_mp3 codec
  • added PCM-exact seeking through the use of a cached lookup table
• replaced dependence on SDL_Sound WAV codec with more robust built-in dr_wav codec
• added Opus handling via pkg-config instead of internal handling via Makefile
  • allows build-compatibility with VisualStudio (with all codecs enabled)

Changes have been build-and-play tested on the following operating systems: Windows 10, Ubuntu Linux 18.04 and 19.04, Rasbian; as well as x86_64, i686, ARM Cortex-A53, PowerPC 7400 (Big-Endian) architectures.

[kcgen]

Says it's waiting for two CI jobs, but they've all passed. Seems like a bug?

[dreamer]

This commit affected build names, and those 2 builds were marked as "required to pass" in branch configuration - I unchecked them for now.

In normal circumstances I like kernel and git project style of review/accepting patches, that forces split into ~one commit per function, but these are not normal circumstances - and we don't have manpower to do this perfectly.

I think first order of business would be to split-out bundled libraries to be introduced each in separate commit (because +53k lines of code in one commit is rather overwhelming), preserving original author and mentioning the exact release (or commit) of upstream project, that was used. I know it might seem a bit pedantic, but this way git-blame information will be preserved, and if we'll need to introduce some changes in these files, it will be easier to resolve conflicts with upstream (if they'll ever happen).

So, my suggestion is: let's create new pull request, with following commits:

• 3 commit introducing only files from dr_libs repo, explicitly setting author info to the lead dev of that project and mentioning exact release in commit message. My suggestion would be to take the files anew, straight from dr_libs repo - this way if some dosbox-specific changes are needed, it will be apparent to future developers by following history of the files with git-log or gitk. Use the same file location as it is in this PR. Licensing on these libs do not require us to distribute license file in binary packages, so we don't need to worry about this for now. (these libs are ~+21k lines of code)
• 1 commit for files related to stb_vorbis. I have some questions regarding this library, but they likely can be trivially explained - let's leave it for that new PR. Same as previous one, licensing does not require us to distribute document with license text. (That would be ~5k lines?)
• 1 commit for files originating from xxHash repo. This one is more complicated regarding to license… Upstream license in repo uses dual-licensing (BSD 2-clause or GPLv2), but the files in this PR (and same files in upstream) specify only BSD 2-clause. This means that eventually we should add a file with BSD license to the files for make install and make dist (after the lib will be hooked in the buildsystem).
• 1 commit for archive.h. Thanks to your comment from other file I know it comes from: https://github.com/voidah/archive, but it would be really nice to have this info in commit message.
• let's not include source code for SDL_sound yet - I tried correlating your changes to the history of SDL_sound sources stored in Mercurial, but couldn't find a revision that you based your changes on. I think you included some DOSBox-specific changes in there as well? SDL2_sound changed license to zlib nowadays, but it does not affect your code here.
• I think there are some other trivial changes, that could be included as well - I will point them out in initial review.

No buildsystem changes, just source of those libs for now. That review could be merged to master quite easily.

After that new PR will be merged, let's rebase this one and continue with proper review. I will leave some comments in code review now, but those are not going to be exhaustive.

[dreamer]

Some initial comments, I will get back to this review later.

note to me: 14/50 files

[kcgen]

I like this approach; we will start from first principles and add layers of the change one merge at a time.
Thanks for the explicit guidance on this; first commit of just the libs is ready!
We can continue to use this merge request as a discussion area to help me prep each layer.

[dreamer]

OK, first 2 parts are merged in and I included a change from SVN, that Qbix implemented few days ago - I think it's time to rebase commit(s) in this review on top of current master branch.

[kcgen]

OK, first 2 parts are merged in and I included a change from SVN, that Qbix implemented few days ago - I think it's time to rebase commit(s) in this review on top of current master branch.

Right on - let me know what you're thinking for the next PR and the commits!

[dreamer]

At +7.6k lines it's much easier to review code already - previously GitHub interface was a bit slow :)

I think the next part, that could be easily merged would probably be build scripts + build.md and CI changes (I think these changes are big enough to put them in separate commits).

After CI, another part could be a dump of original files from SDL_audio (before your changes, with authorship set to Ryan C. Gordon). I think these will be only 3-5 files at this point? (SDL_sound.h, SDL_sound.c, SDL_sound_internal.h and license). There's also file audio_convert.c - but I'm not sure if it was part of SDL or did you include it later? I will try again to find out what was the original revision that you based your changes on.

[dreamer]

Update to my previous comment from 7h ago (since GitHub so helpfully decided to hide it):
I found exact version of SDL_sound you based your changes on: #20

[kcgen]

@dreamer , not many files left in the CD Audio set!
For the next PR, I could add the SDL decoders sources (opus.c, vorbis.c, mp3.cpp and the fast seek table, ...).
That would leave the last PR with the few changes tying it into to the existing DOSBox sources like cdrom_image and the header, configure.ac and the two Makefile.am files.

[dreamer]

@dreamer , not many files left in the CD Audio set!
For the next PR, I could add the SDL decoders sources (opus.c, vorbis.c, mp3.cpp and the fast seek table, ...).
That would leave the last PR with the few changes tying it into to the existing DOSBox sources like cdrom_image and the header, configure.ac and the two Makefile.am files.

Sounds like a plan :)

I am planning to send 2nd set of small patches to upstream (hopefully tonight) - do you want me to include changes, that were merged to src/libs, or do you prefer to send them yourself?

[kcgen]

I like the idea of you sending patches from master, just from a consistency standpoint (ie: Generated the same way, you will include them all as attachments in one message, and Qbix and others can step through one by one).

That said, I want to take any blame and account for any reasoning behind my code, so I want to be fully accountable.

Also, if you think this layered approach to reviewIng and accepting also makes sense for upstream as well (I believe it will), then it makes sense to get the ball rolling now with the audio updates - one by one, so they can feel comfortable and not overwhelmed with the changes.

So yes, would definitely appreciate this!

[dreamer]

I wonder how many lines are left in this review after rebase to the newest master :)

[kcgen]

I wonder how many lines are left in this review after rebase to the newest master :)
Not many! We're about to find out :-)

[dreamer]

@krcroft I just submitted non-CI related commits as a zipfile with patches to sourceforge: https://sourceforge.net/p/dosbox/patches/284/ - please take part in discussion, once it'll start :)

[kcgen]

Thank you for this. Your submission looks great, and I'm looking forward to the discussion.

[dreamer]

Part 4 is merged… I think the bulk part of code, that constituted this review is in? Perhaps it's time to close this PR?

Part 5 covers #27, right?

[kcgen]

Part 4 is merged… I think the bulk part of code, that constituted this review is in? Perhaps it's time to close this PR?

Part 5 covers #27, right?

Yes to both; Part 4 completes the work and covers the patch as it's been (functionally) for quite some time (tested heavily under ECE). It also stays within the confines of how SDL_Sound, the Mixer, and CDROM image "traditionally" operate.

With part 5, all three aspects are now being reshaped to suite our own needs and cut out lots of intermediate code/buffers/behavior.