mamedev / mame

MAME
https://www.mamedev.org/
Other
7.8k stars 1.96k forks source link

`-wavwrite` output seems to differ on each run #10414

Closed firewave closed 1 year ago

firewave commented 1 year ago

I generated two .wav files by running mame -str 2 -wavwrite wavwrite.wav -novideo bublbobl.

Average speed: 99.98% (1 seconds) wavwrite.wav.zip

Average speed: 100.05% (1 seconds) wavwrite2.wav.zip

Then I used https://github.com/bbc/audiowaveform to generate waveforms from the files using audiowaveform -i wavwrite.wav -o wavwrite.png --split-channels --no-axis-labels:

wavwrite11

wavwrite22

After that I used pngcmp to highlight the difference.

diff

firewave commented 1 year ago

Here's images with the highest possible resolution using audiowaveform -i wavwrite.wav -o wavwrite.png --split-channels --no-axis-labels -w 48000 -z auto.

The images are very big so you need to click on them to open them and click again to 100% zoom them. Unfortunately are a bit washed out. I haven't figured out how to make them clearer yet).

wavwrite

wavwrite2

diff

firewave commented 1 year ago

audiowaveform also allows to output the values into a JSON file which makes comparison much easier.

At first it seemed they were just slightly off but there's also some values which are way off. So it doesn't seem like just some minor precision issue.

cd4053 commented 1 year ago

This is my run at it, waveform generated with ffmpeg for windows version: 2022-10-17-git-3bd0bf76fb (gitfull) from here. OS: Windows 10 mame: 0.248 (mame0248-175-gd0bfae953a5)

waveform command: ffmpeg -i input.wav -hide_banner -lavfi "showwavespic=split_channels=1:s=1280x720:colors=0971CE:draw=full:filter=peak" -frames:v 1 output.png

Test 1 - reference, mame defaults mame -str 2 -norc -rompath D:\mame\roms -wavwrite test1.wav -video none bublbobl test1.zip test1 SHA1: b4b09f966d8cbcfa77e371965da7d87a1c5a0766

Test 2 - no change, mame defaults mame -str 2 -norc -rompath D:\mame\roms -wavwrite test2.wav -video none bublbobl test2.zip test2 SHA1: b4b09f966d8cbcfa77e371965da7d87a1c5a0766

Test 3 - different sample rate 44100, changes detected mame -str 2 -norc -rompath D:\mame\roms -wavwrite test3.wav -video none -sr 44100 bublbobl test3.zip test3 SHA1: b3b765f0bb1935a0f94995ee4bd24eaf4ad874d0 <--

Test 4 - different sample rate 44100, changes detected mame -str 2 -norc -rompath D:\mame\roms -wavwrite test3.wav -video none -sr 44100 bublbobl test4.zip test4 SHA1: 5412a8a81dff307c2335ef17491771aeb9cdda3e <--

firewave commented 1 year ago

Thanks for the ffmpeg command. I have been trying to generate one with it a while back but I didn't succeed,

BTW I was using the official MAME 0.248 Windows 64-bit binary on Windows 10. And I used audiowaveform 1.6.0.

jflatt commented 1 year ago

I just tried this on 0.249 and produced correct results. Ran the same command twice, opened one file in Audacity, imported the second file, inverted second file, mixed the tracks and it produced silence. Exported that to headerless raw file and inspected it in a hex editor, the file was all zeroes.

firewave commented 1 year ago

@jflatt That's an interesting approach. Thanks for sharing.

MooglyGuy commented 1 year ago

Taking SHA1 hashes, as well as comparing inverted waveforms, or doing file compares, is the way to go if you want a ground-truth comparison. There's no guarantee a third-party application itself doesn't have some fault in terms of inconsistent rendering of waveforms, so comparing the PNGs could produce either false-negative matches or false-positive deltas.

I'm curious about the hashes posted by @cd4053 - the "MAME defaults" runs have identical hashes, so the file contents are almost guaranteed to be identical. I would expect the two runs of 44100Hz to also be identical, but those two hash files don't match!

So you definitely are onto something here. The two "MAME defaults" runs should match - and do - but the two -sr 44100 runs should also match each other - and don't.

firewave commented 1 year ago

Taking SHA1 hashes, as well as comparing inverted waveforms, or doing file compares, is the way to go if you want a ground-truth comparison. There's no guarantee a third-party application itself doesn't have some fault in terms of inconsistent rendering of waveforms, so comparing the PNGs could produce either false-negative matches or false-positive deltas.

I attached the *.wav files from my runs and they are clearly different by content. The visualization is just that - a visualization. Otherwise it is quite hard to "diff" a soundfile.

I do know that what is being output by the underlying audio system on the operating system might lead to differences. But I expected the "raw" output to be consistent so I wanted to get some feedback on this if my assumption was right.

I also did not come up with other examples yet since a lot of systems are silent by default or take a while until the music play so my random picks with limited runtime come up with only silence. Also Bubble Bobble has some of my favourite music :)

MooglyGuy commented 1 year ago

Disregard the comment that I left a few minutes ago. I was able to reproduce the issue. Interestingly, so far I've only been able to reproduce it on systems that use the Yamaha YM3526 - in my testing, I wasn't able to reproduce it with:

But I can reproduce it with 'Wheels Runner' (wheelrun).

firewave commented 1 year ago

Thanks for looking into this.

Good to know it doesn't seem to be a general issue. Maybe it's just uninitialized (or intentionally randomized) memory which leads to the differences. As seen in the visual diff it's still very similar and not completely off. Using valgrind would be sufficient to detect this. Unfortunately the sanitized binaries are too big to run them with it (if that is even possible) and those are the only ones I have currently at hand.

firewave commented 1 year ago

It is uninitialized memory alright.

Running -wavwrite wavwrite.wav bublbol produces a truckload of messages in valgrind originating from the ym2203_device: bublbobl.valgrind.txt

This probably applies to the other ymfm devices as well.

Also simply running -validate produces the following (at least - still in progress):

==29778== Conditional jump or move depends on uninitialised value(s)
==29778==    at 0x1932C97B: ymfm::fm_engine_base<ymfm::opl_registers_base<3> >::assign_operators() (ymfm_fm.ipp:1444)
==29778==    by 0x19322E6A: ymfm::fm_engine_base<ymfm::opl_registers_base<3> >::fm_engine_base(ymfm::ymfm_interface&) (ymfm_fm.ipp:1204)
==29778==    by 0x1931B445: ymfm::ymf262::ymf262(ymfm::ymfm_interface&) (../../../../../3rdparty/ymfm/src/ymfm_opl.cpp:1252)
==29778==    by 0x151FFDAB: ymfm_device_base<ymfm::ymf262>::ymfm_device_base(machine_config const&, char const*, device_t*, unsigned int, emu::detail::device_type_impl_base const&) (ymfm_mame.h:183)
==29778==    by 0x151FD7FF: ymf262_device::ymf262_device(machine_config const&, char const*, device_t*, unsigned int) (../../../../../src/devices/sound/ymopl.cpp:110)
==29778==    by 0x15206830: std::__detail::_MakeUniq<ymf262_device>::__single_object std::make_unique<ymf262_device, machine_config const&, char const*&, device_t*&, unsigned int&>(machine_config const&, char const*&, device_t*&, unsigned int&) (unique_ptr.h:1065)
==29778==    by 0x15206773: std::unique_ptr<device_t, std::default_delete<device_t> > emu::detail::device_type_impl_base::create_device<ymf262_device>(emu::detail::device_type_impl_base const&, machine_config const&, char const*, device_t*, unsigned int) (device.h:204)
==29778==    by 0x15E9D6A2: emu::detail::device_type_impl_base::create(machine_config const&, char const*, device_t*, unsigned int) const (device.h:281)
==29778==    by 0x1840B5F6: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1157E2DA: device_t* machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, unsigned int) (mconfig.h:187)
==29778==    by 0x1157E24A: auto machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, XTAL) (mconfig.h:203)
==29778==    by 0x1157B4E2: ymf262_device& emu::detail::device_type_impl<ymf262_device>::operator()<XTAL>(machine_config&, char const*, XTAL&&) const (device.ipp:36)
==29778==    by 0x12AE0975: sb16_lle_device::device_add_mconfig(machine_config&) (../../../../../src/devices/bus/isa/sb16.cpp:431)
==29778==    by 0x169253C5: device_t::add_machine_configuration(machine_config&) (../../../../../src/emu/device.cpp:226)
==29778==    by 0x1840BD64: machine_config::add_device(std::unique_ptr<device_t, std::default_delete<device_t> >&&, device_t*) (../../../../../src/emu/mconfig.cpp:316)
==29778==    by 0x1840B607: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1851C63C: validity_checker::validate_devices(machine_config&) (../../../../../src/emu/validity.cpp:2588)
==29778==    by 0x18511857: validity_checker::validate_one(game_driver const&) (../../../../../src/emu/validity.cpp:1955)
==29778==    by 0x18511DBA: validity_checker::check_all_matching(char const*) (../../../../../src/emu/validity.cpp:1864)
==29778==    by 0x16778795: cli_frontend::execute_commands(std::basic_string_view<char, std::char_traits<char> >) (../../../../../src/frontend/mame/clifront.cpp:1700)
==29778==    by 0x16778006: cli_frontend::start_execution(mame_machine_manager*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (../../../../../src/frontend/mame/clifront.cpp:248)
==29778==    by 0x16779538: cli_frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/clifront.cpp:291)
==29778==    by 0x158A4D65: emulator_info::start_frontend(emu_options&, osd_interface&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/mame.cpp:454)
==29778==    by 0x18558061: main (../../../../../src/osd/sdl/sdlmain.cpp:191)
==29778==  Uninitialised value was created by a heap allocation
==29778==    at 0x1C480F01: operator new(unsigned long) (vg_replace_malloc.c:434)
==29778==    by 0x152067D3: std::__detail::_MakeUniq<ymf262_device>::__single_object std::make_unique<ymf262_device, machine_config const&, char const*&, device_t*&, unsigned int&>(machine_config const&, char const*&, device_t*&, unsigned int&) (unique_ptr.h:1065)
==29778==    by 0x15206773: std::unique_ptr<device_t, std::default_delete<device_t> > emu::detail::device_type_impl_base::create_device<ymf262_device>(emu::detail::device_type_impl_base const&, machine_config const&, char const*, device_t*, unsigned int) (device.h:204)
==29778==    by 0x15E9D6A2: emu::detail::device_type_impl_base::create(machine_config const&, char const*, device_t*, unsigned int) const (device.h:281)
==29778==    by 0x1840B5F6: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1157E2DA: device_t* machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, unsigned int) (mconfig.h:187)
==29778==    by 0x1157E24A: auto machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, XTAL) (mconfig.h:203)
==29778==    by 0x1157B4E2: ymf262_device& emu::detail::device_type_impl<ymf262_device>::operator()<XTAL>(machine_config&, char const*, XTAL&&) const (device.ipp:36)
==29778==    by 0x12AE0975: sb16_lle_device::device_add_mconfig(machine_config&) (../../../../../src/devices/bus/isa/sb16.cpp:431)
==29778==    by 0x169253C5: device_t::add_machine_configuration(machine_config&) (../../../../../src/emu/device.cpp:226)
==29778==    by 0x1840BD64: machine_config::add_device(std::unique_ptr<device_t, std::default_delete<device_t> >&&, device_t*) (../../../../../src/emu/mconfig.cpp:316)
==29778==    by 0x1840B607: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1851C63C: validity_checker::validate_devices(machine_config&) (../../../../../src/emu/validity.cpp:2588)
==29778==    by 0x18511857: validity_checker::validate_one(game_driver const&) (../../../../../src/emu/validity.cpp:1955)
==29778==    by 0x18511DBA: validity_checker::check_all_matching(char const*) (../../../../../src/emu/validity.cpp:1864)
==29778==    by 0x16778795: cli_frontend::execute_commands(std::basic_string_view<char, std::char_traits<char> >) (../../../../../src/frontend/mame/clifront.cpp:1700)
==29778==    by 0x16778006: cli_frontend::start_execution(mame_machine_manager*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (../../../../../src/frontend/mame/clifront.cpp:248)
==29778==    by 0x16779538: cli_frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/clifront.cpp:291)
==29778==    by 0x158A4D65: emulator_info::start_frontend(emu_options&, osd_interface&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/mame.cpp:454)
==29778==    by 0x18558061: main (../../../../../src/osd/sdl/sdlmain.cpp:191)
==29778== Use of uninitialised value of size 8
==29778==    at 0x1932DAF5: std::__uniq_ptr_impl<ymfm::fm_operator<ymfm::opl_registers_base<3> >, std::default_delete<ymfm::fm_operator<ymfm::opl_registers_base<3> > > >::_M_ptr() const (unique_ptr.h:191)
==29778==    by 0x1932D994: std::unique_ptr<ymfm::fm_operator<ymfm::opl_registers_base<3> >, std::default_delete<ymfm::fm_operator<ymfm::opl_registers_base<3> > > >::get() const (unique_ptr.h:462)
==29778==    by 0x1932C9A5: ymfm::fm_engine_base<ymfm::opl_registers_base<3> >::assign_operators() (ymfm_fm.ipp:1444)
==29778==    by 0x19322E6A: ymfm::fm_engine_base<ymfm::opl_registers_base<3> >::fm_engine_base(ymfm::ymfm_interface&) (ymfm_fm.ipp:1204)
==29778==    by 0x1931B445: ymfm::ymf262::ymf262(ymfm::ymfm_interface&) (../../../../../3rdparty/ymfm/src/ymfm_opl.cpp:1252)
==29778==    by 0x151FFDAB: ymfm_device_base<ymfm::ymf262>::ymfm_device_base(machine_config const&, char const*, device_t*, unsigned int, emu::detail::device_type_impl_base const&) (ymfm_mame.h:183)
==29778==    by 0x151FD7FF: ymf262_device::ymf262_device(machine_config const&, char const*, device_t*, unsigned int) (../../../../../src/devices/sound/ymopl.cpp:110)
==29778==    by 0x15206830: std::__detail::_MakeUniq<ymf262_device>::__single_object std::make_unique<ymf262_device, machine_config const&, char const*&, device_t*&, unsigned int&>(machine_config const&, char const*&, device_t*&, unsigned int&) (unique_ptr.h:1065)
==29778==    by 0x15206773: std::unique_ptr<device_t, std::default_delete<device_t> > emu::detail::device_type_impl_base::create_device<ymf262_device>(emu::detail::device_type_impl_base const&, machine_config const&, char const*, device_t*, unsigned int) (device.h:204)
==29778==    by 0x15E9D6A2: emu::detail::device_type_impl_base::create(machine_config const&, char const*, device_t*, unsigned int) const (device.h:281)
==29778==    by 0x1840B5F6: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1157E2DA: device_t* machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, unsigned int) (mconfig.h:187)
==29778==    by 0x1157E24A: auto machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, XTAL) (mconfig.h:203)
==29778==    by 0x1157B4E2: ymf262_device& emu::detail::device_type_impl<ymf262_device>::operator()<XTAL>(machine_config&, char const*, XTAL&&) const (device.ipp:36)
==29778==    by 0x12AE0975: sb16_lle_device::device_add_mconfig(machine_config&) (../../../../../src/devices/bus/isa/sb16.cpp:431)
==29778==    by 0x169253C5: device_t::add_machine_configuration(machine_config&) (../../../../../src/emu/device.cpp:226)
==29778==    by 0x1840BD64: machine_config::add_device(std::unique_ptr<device_t, std::default_delete<device_t> >&&, device_t*) (../../../../../src/emu/mconfig.cpp:316)
==29778==    by 0x1840B607: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1851C63C: validity_checker::validate_devices(machine_config&) (../../../../../src/emu/validity.cpp:2588)
==29778==    by 0x18511857: validity_checker::validate_one(game_driver const&) (../../../../../src/emu/validity.cpp:1955)
==29778==    by 0x18511DBA: validity_checker::check_all_matching(char const*) (../../../../../src/emu/validity.cpp:1864)
==29778==    by 0x16778795: cli_frontend::execute_commands(std::basic_string_view<char, std::char_traits<char> >) (../../../../../src/frontend/mame/clifront.cpp:1700)
==29778==    by 0x16778006: cli_frontend::start_execution(mame_machine_manager*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (../../../../../src/frontend/mame/clifront.cpp:248)
==29778==    by 0x16779538: cli_frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/clifront.cpp:291)
==29778==    by 0x158A4D65: emulator_info::start_frontend(emu_options&, osd_interface&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/mame.cpp:454)
==29778==    by 0x18558061: main (../../../../../src/osd/sdl/sdlmain.cpp:191)
==29778==  Uninitialised value was created by a heap allocation
==29778==    at 0x1C480F01: operator new(unsigned long) (vg_replace_malloc.c:434)
==29778==    by 0x152067D3: std::__detail::_MakeUniq<ymf262_device>::__single_object std::make_unique<ymf262_device, machine_config const&, char const*&, device_t*&, unsigned int&>(machine_config const&, char const*&, device_t*&, unsigned int&) (unique_ptr.h:1065)
==29778==    by 0x15206773: std::unique_ptr<device_t, std::default_delete<device_t> > emu::detail::device_type_impl_base::create_device<ymf262_device>(emu::detail::device_type_impl_base const&, machine_config const&, char const*, device_t*, unsigned int) (device.h:204)
==29778==    by 0x15E9D6A2: emu::detail::device_type_impl_base::create(machine_config const&, char const*, device_t*, unsigned int) const (device.h:281)
==29778==    by 0x1840B5F6: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1157E2DA: device_t* machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, unsigned int) (mconfig.h:187)
==29778==    by 0x1157E24A: auto machine_config::device_add<emu::detail::device_type_impl<ymf262_device> const&>(char const*, emu::detail::device_type_impl<ymf262_device> const&, XTAL) (mconfig.h:203)
==29778==    by 0x1157B4E2: ymf262_device& emu::detail::device_type_impl<ymf262_device>::operator()<XTAL>(machine_config&, char const*, XTAL&&) const (device.ipp:36)
==29778==    by 0x12AE0975: sb16_lle_device::device_add_mconfig(machine_config&) (../../../../../src/devices/bus/isa/sb16.cpp:431)
==29778==    by 0x169253C5: device_t::add_machine_configuration(machine_config&) (../../../../../src/emu/device.cpp:226)
==29778==    by 0x1840BD64: machine_config::add_device(std::unique_ptr<device_t, std::default_delete<device_t> >&&, device_t*) (../../../../../src/emu/mconfig.cpp:316)
==29778==    by 0x1840B607: machine_config::device_add(char const*, emu::detail::device_type_impl_base const&, unsigned int) (../../../../../src/emu/mconfig.cpp:201)
==29778==    by 0x1851C63C: validity_checker::validate_devices(machine_config&) (../../../../../src/emu/validity.cpp:2588)
==29778==    by 0x18511857: validity_checker::validate_one(game_driver const&) (../../../../../src/emu/validity.cpp:1955)
==29778==    by 0x18511DBA: validity_checker::check_all_matching(char const*) (../../../../../src/emu/validity.cpp:1864)
==29778==    by 0x16778795: cli_frontend::execute_commands(std::basic_string_view<char, std::char_traits<char> >) (../../../../../src/frontend/mame/clifront.cpp:1700)
==29778==    by 0x16778006: cli_frontend::start_execution(mame_machine_manager*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) (../../../../../src/frontend/mame/clifront.cpp:248)
==29778==    by 0x16779538: cli_frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/clifront.cpp:291)
==29778==    by 0x158A4D65: emulator_info::start_frontend(emu_options&, osd_interface&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&) (../../../../../src/frontend/mame/mame.cpp:454)
==29778==    by 0x18558061: main (../../../../../src/osd/sdl/sdlmain.cpp:191)

CC'ing @aaronsgiles.

aaronsgiles commented 1 year ago

Erm, that's not from ym2203_device since it's OPL, and in fact it's the ymf262_device from the sb16.

Yes, there is a use of uninitialized memory there, in that m_regdata isn't cleared until reset time, but the base class is calling assign_operators() at construction time. But I'm dubious that's the cause of any problems, since assign_operators() will get called on each clock before sound generation.

Anyway, if you want to silence this warning and see if I'm wrong about my supposition, you can modify the opl_register_base constructor in ymfm_opl.cpp to clear regdata in the constructor, as follows:

template<int Revision>
opl_registers_base<Revision>::opl_registers_base() :
    m_lfo_am_counter(0),
    m_lfo_pm_counter(0),
    m_noise_lfsr(1),
    m_lfo_am(0),
    m_regdata{ 0 }

OPL3 is the only case where this should be necessary, since it's the only variant I've done with dynamic operator mapping.

aaronsgiles commented 1 year ago

Ok, I see I missed the first part with the attached .txt file.

Based on that, it seems to think that the duration_in_clocks parameter passed to ymfm_set_timer is based on uninitialized data.

The caller fm_engine_base<ymfm::opn_registers_base<false>>::update_timer computes that value from m_clock_prescale (which is initialized in the constructor) and either m_regs.timer_a_value() or m_regs.timer_b_value(). Both of those values are dependent upon the values in the m_regs array.

However m_regs is explicitly filled with 0 during opn_registers_base<>::reset(), so unless reset is not getting called, I don't see how anything can be uninitialized down this path.

firewave commented 1 year ago

Thanks for the fast reply and analysis.

Looking at the first from the log and reviewing the code it seems like fm_engine_base::m_total_clocks is uninitialized and the usage starts in fm_engine_base<RegisterType>::engine_mode_write() with:

update_timer(1, m_regs.load_timer_b(), -(m_total_clocks & 15));

For some reason debugging fails with either gdb or lldb but adding printf debugging confirms this with the values being different on every execution and having identical values after initializing it in fm_engine_base<RegisterType>::fm_engine_base(ymfm_interface &intf). There's also no more valgrind messages.

Sorry for not providing this info immediately with the log but it was late. 😐😴

I have not looked at the other one yet.

firewave commented 1 year ago

The WAV files are now also identical on each run.

And I can confirm that the otherwise proposed change fixes the messages during -validate. Thanks.

aaronsgiles commented 1 year ago

Ah, I see now. I should have looked one level back. Yes, it appears that m_total_clocks is not being initialized. Thanks for the helpful analysis.

aaronsgiles commented 1 year ago

Ok, I pushed an update to ymfm to fix this. This is probably a reasonable time to re-sync and pick up a couple of other small fixes. Once I can verify a build, I'll create a PR to sync.

cuavas commented 1 year ago

Should be addressed by #10583.

firewave commented 1 year ago

Thanks a lot!

Ah, I see now. I should have looked one level back. Yes, it appears that m_total_clocks is not being initialized. Thanks for the helpful analysis.

The valgrind warnings are sometimes not straight-forward and it doesn't report the first immediately usage of the uninitialized value requiring a bit of backtracking. Years ago when I was fixing these issues by the dozen I usually fired up Visual Studio which has very nice memory coloring in debug build which greatly helps identifying these issues.

Should be addressed by #10583.

Confirmed.