#284 Prerequisites for SDL_sound replacement and SDL2, plus some other changes
This is the second patch series from dosbox-staging repo. Zip file contains 20 patches, each patch includes detailed explanation in commit message and authorship information pointing to the original author.
Summary:
- Update to distclean makefile target (2 patches)
- Add prerequisite libraries needed for krcroft's patch, 1 library per commit: dr_flac, dr_mp3, dr_wav, stb_vorbis, xxHash, archive (C++ object serializer), and subset of necessary files from SDL_sound. These commits only include the library files - they do not hook them up in buildsystem yet - if included, it will make it easier to review and merge-in rest of krcroft's patch later on. (7 patches)
- krcroft's changes to SDL_sound library necessary for future usage in DOSBox (6 patches)
- Remove of dependency on 2 macros from SDL_cdrom library - this is prerequisite for future SDL2 work (2 patches)
- Silence compiler warnings introduced in r4277
- Prevent potential null pointer dereference in dynamic core code - fixes 1 issue reported by Clang static analyzer
- Add DOSBox splash screen graphic in SVG format
Patches are numbered in order of application. They were generated using command: git format-patch 9099f..1f16a ^svn/trunk, and CI and Git related commits removed.
Links to exact commits, as it might be easier to read them:
27995003 Augment the generate distclean target (fixes #14)
0b221118 Remove one duplicate and arange directories after files
a5d87fa8 Add built-in FLAC codec: dr_flac v0.12.2
a5f1e347 Add built-in MP3 codec: dr_mp3 v0.5.1
d66317a0 Add built-in WAV codec: dr_wav v0.11.1
fd5aeab8 Add built-in Vorbis codec: stb_vorbis v1.17
28cf402c Add built-in Extremely Fast Hash algorithm: xxHash [snapshot as of 2019-10-23]
db67ff2e Add built-in C++ object serializer [snapshot as of 2019-10-23]
0420181d Add built-in SDL_sound r599
d2333979 Tailor SDL_Sound for interal use by DOSBox
d9658b47 Replace strncpy with snprintf
b99184d9 Replace SDL_RWFromMem with more applicable SDL_RWFromConstMem
5736a7de Make comment match reality
1f193e14 Simplify the task of making bit-field operations C and C++ compatible
f39b51fa Add SDL decoders for their corresponding codecs
fce61e33 Implement frames/MSF conversion as functions
4af8459e Replace MSF_TO_FRAMES, FRAMES_TO_MSF macros
24872420 Silence compiler warnings introduced in r4277
4a01b6a0 Prevent potential null pointer dereference
1f16a164 Add DOSBox splash screen graphic in vector format
All these commits were reviewed, reviews can be found by browsing https://github.com/dreamer/dosbox-staging/pulls?q=is%3Apr+is%3Aclosed
None of patches from patch series 2 was applied to SVN yet, but in the meantime, we had a lot of work done in dosbox-staging. Attaching patch series 3.
These are the commits: https://github.com/dreamer/dosbox-staging/compare/a183237d0707%5E...ee54da6ee765
They depend on changes introduced in patch series 2.
Short summary of changes:
Some of changes we made would be very hard or impossible to implement without C++11, therefore this patch tries to enable it again. We need this.
Thank you for the patches.
For some of the patches, I am really not sure what to do with them. Like the dist-clean patch. That sounds like actually making things more complicated as then there is yet another thing to update if something gets added. What needs this ? Do you have an actual use case for this ?
Another thing that I am confused about is the splashscreen art. Why was that put under a difference license and with different holders(agents?) ? Whom would be people need to attribute ? What benefit does that bring us (the different format/not being the copyright holders anymore) ?
About the nullpointer thing. It is slower as an extra compare is added and according to jmarsh it is not bug, but the analyzer being wrong, why should I slow down dosbox in order to fix a wrong warning ?
We can go in details and iterate on these patches until they are ok for inclusion, or just deemed unacceptable.
I believe this will explain: https://github.com/dreamer/dosbox-staging/issues/14
Personally, I use
git clean -fdxfor this purpose, but users receiving the tarball with the code (e.g. from their OS distribution or as source code distributed with the game) won't have such option.The file listing was later revised in 2909ca but it seems it got ommitted from patch series (most likely because it touched a .gitignore file, and I send only patches, that apply cleanly to SVN). Let's put this patch aside for now - we'll squash all commits relevant to this change in a new one and include in revised version of "patch series 2".
Ah, I suspected I was not thorough enough in writing the commit message. (commit
messages are included inside the patch files btw, you don't need to look them up
on GitHub).
Because it is not source code and GPL is not appropriate for artwork. I was not sure what to do about it, so I downloaded various svg images used by GNU project (e.g. logo for GPLv3 license) and looked up what licence they use for graphical assets. I am open to changing it (but I'm not sure how to correctly fill metadata for svg image using GPL - Inkscape does not have such preset, only CC* licenses) or leave it blank.
Huh? It is set to:
- Creator: GOG Team (this is the best info about original image I could find in repo - in THANKS file) … should I put "The DOSBox Team" in there? It would make sense somewhat.
- Contributors: me (I spent quite a lot of time optimizing this image to use the fewest Bezier curves possible, matching the image to be nearly pixel-perfect recreation (when rendered at the same size) and solving peculiarities of svg antialiasing rendering…
Creator, I presume - e.g. by not removing metadata.
Ability to revise/update the splash screen, e.g. to make it render nicely in fullscreen or on high-dpi screens. It can also be used for icons or artwork on the website or whatever is necessary in the future…
I presume it's about 4a01b6.
Before just implementing the nullcheck, I tried to convince static analyzer,
that this is not an issue via asserts, and that grew quite complicated. No matter
what I did, analyzer was pointing out, that the issue still exists.
And by looking at this function with fresh eye, I still think (still not 100% sure)
it might happen, e.g. if gen_synchreg is called twice in a row with the same
pointer passed as first parameter:
- first call invokes dnew->genreg->Clear(), which sets null and unsets DYNFLG_CHANGED to prevent null ptr dereference, but flag is immediately set again
- second call still has null pointer from first invocation, with DYNFLG_CHANGED indicating, that it's safe to dereference
Overall this code is really, really hard to understand.
With GCC_LIKELY in there? Do you know what benchmark I can run to verify if this
change affects the performance in any measurable way?
Regarding distclean, the patched version gets rid of all the derived and generated autotools files, which is useful when diffing between source trees.
For example, even the tarball here - http://source.dosbox.com/dosboxsvn.tgz already comes with the following generated autotools files; and there's no way to strip them out using any of the make clean targets. These can all trigger false-positive diff results, which have to be manually excluded from the diff or the files manually deleted prior to diffing.
aclocal.m4
compile
config.guess
config.h.in
config.sub
configure
depcomp
install-sh
Makefile.in
missing
docs/Makefile.in
include/Makefile.in
src/Makefile.in
visualc_net/Makefile.in
Uploaded patch series 4.
It consists of all the commits from series 2, 3 and all new changes since then (in total: 78 patches); content can be also browsed on GitHub: https://github.com/dreamer/dosbox-staging/compare/317861791151%5E...b86c178e67d0
New patches in the series fix several small bugs, resource leaks, do general cleanup enabled by C++11 usage (relevant bugs and usecases are mentioned in the commit messages embedded in the patches).
I think all the fixes that improved Coverity finding on master branch are included: https://scan.coverity.com/projects/dosbox-staging