mickelson / attract

A graphical front-end for command line emulators that hides the underlying operating system and is intended to be controlled with a joystick or gamepad.
http://attractmode.org
GNU General Public License v3.0
393 stars 113 forks source link

Crash when cycling through games when using video #735

Closed spilinek closed 1 year ago

spilinek commented 1 year ago

Ubuntu 20.04.1

Compiled latest attract code Layout: At-The-Arcade from The Great Themes Collection v10.3.2

Compiled latest attractplus code Layout: At-The-Arcade from The Great Themes Collection Plus Beta 2

attract and attractplus both crash when cycling through games list while using:

Below are a couple examples of crashes I'm seeing. I think there were other variations of errors reported when the crash happened.

double free or corruption (!prev)
Aborted (core dumped)
malloc(): corrupted top size
Aborted (core dumped)

Compiled the latest version of Valgrind following these instructions

Started attract with valgrind: ~/valgrind/vg-in-place --leak-check=yes ~/latest_attract/attract --loglevel debug 2>&1 | tee ~/latest_attract/log.txt

While using valgrind, navigation in attract was very slow. I couldn't reproduce the crash, but I did capture invalid writes. For example:

==30343== Invalid write of size 8
==30343==    at 0x67C3C03: ??? (in /usr/lib/x86_64-linux-gnu/libswscale.so.5.9.100)
==30343==    by 0x33F0F77F: ???
==30343==    by 0xFC713FF: ???
==30343==  Address 0x35555200 is 691,200 bytes inside a block of size 691,201 alloc'd
==30343==    at 0x484D7E3: memalign (vg_replace_malloc.c:1531)
==30343==    by 0x484D94D: posix_memalign (vg_replace_malloc.c:1704)
==30343==    by 0x64CF9E4: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.56.70.100)
==30343==    by 0x64CA5C9: av_image_alloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.56.70.100)
==30343==    by 0x2C6CF7: FeVideoImp::init_rgba_buffer() (in /home/jeremy/latest_attract/attract)
==30343==    by 0x2C8E37: FeMedia::open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, sf::Texture*) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x248B18: FeTextureContainer::load_with_ffmpeg(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x249BC7: FeTextureContainer::try_to_load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x24ABEF: FeTextureContainer::internal_update_selection(FeSettings*) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x23FE63: FePresent::update(bool, bool) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x23FF8C: FePresent::update_to_new_list(int, bool, bool) (in /home/jeremy/latest_attract/attract)
==30343==    by 0x1C4D0B: main (in /home/jeremy/latest_attract/attract)

This got me looking into FeVideoImp::init_rgba_buffer() in media.cpp. Searched Google for av_image_alloc. I came across this and this where they're using 32 for the align in av_image_alloc. Attract is using 1 for align. I'm not familiar with FFmpeg and couldn't tell what's the difference between 1 and 32 for align.

In media.cpp I located this:

void FeVideoImp::init_rgba_buffer()
{
    std::lock_guard<std::recursive_mutex> l( image_swap_mutex );

    if (rgba_buffer[0])
        av_freep(&rgba_buffer[0]);

    int ret = av_image_alloc(rgba_buffer, rgba_linesize,
            disptex_width, disptex_height,
            AV_PIX_FMT_RGBA, 1);

    if (ret < 0)
        FeLog() << "Error allocating rgba buffer" << std::endl;
}

And switched av_image_alloc() align to 32:

void FeVideoImp::init_rgba_buffer()
{
    std::lock_guard<std::recursive_mutex> l( image_swap_mutex );

    if (rgba_buffer[0])
        av_freep(&rgba_buffer[0]);

    int ret = av_image_alloc(rgba_buffer, rgba_linesize,
            disptex_width, disptex_height,
            AV_PIX_FMT_RGBA, 32);
            // AV_PIX_FMT_RGBA, 1);

    if (ret < 0)
        FeLog() << "Error allocating rgba buffer" << std::endl;
}

With align 32 I'm no longer seeing crashes in attract and attractplus.

Ran through valgrind with my fix and I no longer see invalid writes reported.

mickelson commented 1 year ago

That’s great, can you do a pull request with your fix?  If not no problem, I can patch this when I get a chanceAndrewOn Jan 30, 2023, at 8:44 AM, spilinek @.***> wrote: Ubuntu 20.04.1 Compiled latest attract code Layout: At-The-Arcade from The Great Themes Collection v10.3.2 Compiled latest attractplus code Layout: At-The-Arcade from The Great Themes Collection Plus Beta 2 attract and attractplus both crash when cycling through games list while using:

Up (Previous Game) Down (Next Game) Previous Page (mapped to PageUp key) Next Page (mapped to PageDown key)

Below are a couple examples of crashes I'm seeing. I think there were other variations of errors reported when the crash happened. double free or corruption (!prev) Aborted (core dumped)

malloc(): corrupted top size Aborted (core dumped)

Compiled the latest version of Valgrind following these instructions Started attract with valgrind: ~/valgrind/vg-in-place --leak-check=yes ~/latest_attract/attract --loglevel debug 2>&1 | tee ~/latest_attract/log.txt When using valgrind navigation was very slow in attract. I couldn't reproduce the crash, but I did capture invalid writes. For example: ==30343== Invalid write of size 8 ==30343== at 0x67C3C03: ??? (in /usr/lib/x86_64-linux-gnu/libswscale.so.5.9.100) ==30343== by 0x33F0F77F: ??? ==30343== by 0xFC713FF: ??? ==30343== Address 0x35555200 is 691,200 bytes inside a block of size 691,201 alloc'd ==30343== at 0x484D7E3: memalign (vg_replace_malloc.c:1531) ==30343== by 0x484D94D: posix_memalign (vg_replace_malloc.c:1704) ==30343== by 0x64CF9E4: av_malloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.56.70.100) ==30343== by 0x64CA5C9: av_image_alloc (in /usr/lib/x86_64-linux-gnu/libavutil.so.56.70.100) ==30343== by 0x2C6CF7: FeVideoImp::init_rgba_buffer() (in /home/jeremy/latest_attract/attract) ==30343== by 0x2C8E37: FeMedia::open(std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, sf::Texture) (in /home/jeremy/latest_attract/attract) ==30343== by 0x248B18: FeTextureContainer::load_with_ffmpeg(std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, bool) (in /home/jeremy/latest_attract/attract) ==30343== by 0x249BC7: FeTextureContainer::try_to_load(std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, std::cxx11::basic_string<char, std::char_traits, std::allocator > const&, bool) (in /home/jeremy/latest_attract/attract) ==30343== by 0x24ABEF: FeTextureContainer::internal_update_selection(FeSettings) (in /home/jeremy/latest_attract/attract) ==30343== by 0x23FE63: FePresent::update(bool, bool) (in /home/jeremy/latest_attract/attract) ==30343== by 0x23FF8C: FePresent::update_to_new_list(int, bool, bool) (in /home/jeremy/latest_attract/attract) ==30343== by 0x1C4D0B: main (in /home/jeremy/latest_attract/attract)

This got me looking into FeVideoImp::init_rgba_buffer() in media.cpp. Searched Google for av_image_alloc. I came across this and this where they're using 32 for the align in av_image_alloc. Attract is using 1 for align. I'm not familiar with FFmpeg and couldn't tell what's the difference between 1 and 32 for align. In media.cpp I located this: void FeVideoImp::init_rgba_buffer() { std::lock_guard l( image_swap_mutex );

if (rgba_buffer[0])
    av_freep(&rgba_buffer[0]);

int ret = av_image_alloc(rgba_buffer, rgba_linesize,
        disptex_width, disptex_height,
        AV_PIX_FMT_RGBA, 1);

if (ret < 0)
    FeLog() << "Error allocating rgba buffer" << std::endl;

}

And switched av_image_alloc() align to 32: void FeVideoImp::init_rgba_buffer() { std::lock_guard l( image_swap_mutex );

if (rgba_buffer[0])
    av_freep(&rgba_buffer[0]);

int ret = av_image_alloc(rgba_buffer, rgba_linesize,
        disptex_width, disptex_height,
        AV_PIX_FMT_RGBA, 32);
        // AV_PIX_FMT_RGBA, 1);

if (ret < 0)
    FeLog() << "Error allocating rgba buffer" << std::endl;

}

With align 32 I'm no longer seeing crashes in attract and attractplus. Ran through valgrind with my fix and I no longer see invalid writes reported.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

spilinek commented 1 year ago

Created https://github.com/mickelson/attract/pull/736

spilinek commented 1 year ago

Attractplus has been stable since I made the change of align from 1 to 32.

Side effect: I noticed this caused some videos to become distorted.

I added debug logging in media.cpp to identify why some videos were distorted. I found distorted videos with a width not divisible by 32 were distorted.

854x480

640x480

618x480

960x480

420x480

360x480

The last example is another wrinkle in this problem. The width (360) is not divisible by 32. I did some further digging into this and found that the videos Pixel Format also influences if the video is distorted. Attract is converting all videos to AV_PIX_FMT_RGBA. I added more debug logging to identify the pixel format.

I found that my videos are in two pixel formats (videos scrapped from screenscraper.fr):

I created two rom lists with 10+ entries:

Further testing with Valgrind and I found that AV_PIX_FMT_YUV444P videos don't have invalid writes when align=1

Some AV_PIX_FMT_YUV420P videos display ok with align=32. Some videos only display with align=1, but Valgrind reports invalid writes. Also it's very easy to crash attract with align=1.

Current logic I'm using that appears to be stable and makes the most videos visible:

Default: align=32
If (width % 32 > 0)
    If (px_fmt == AV_PIX_FMT_YUV444P) Then align=1
    If(px_fmt == AV_PIX_FMT_YUV420P) Then align=32

I've tried AV_PIX_FMT_YUV420P with align: 1, 2, 3, 4, 8, 16. Fixes some videos, but some still cause a core dump.

Also tried keeping AV_PIX_FMT_YUV420P as AV_PIX_FMT_YUV420P or converting to AV_PIX_FMT_YUV444P. No crashes, but video is still distorted. Seems attract needs AV_PIX_FMT_RGBA.

(AV_PIX_FMT_RGB32 + align=32) works for some, but not for the same videos that have problem with (AV_PIX_FMT_RGBA + align=32).

All my tests were done with attractplus. I assume the same issue is happening with attract.

mickelson commented 1 year ago

please try again with an updated attract (mot attractplus) and in particular with this commit: https://github.com/mickelson/attract/commit/51d3fefbeeab5dfd5a9dbafce78e25f352e4fe9cI found what may be the same the same issue when testing and fixed with the aboveOn Feb 1, 2023, at 11:22 AM, spilinek @.***> wrote: Attractplus has been stable since I made the change of align from 1 to 32. Side effect: I noticed this caused some videos to become distorted. I added debug logging in media.cpp to identify why some videos were distorted. I found distorted videos with a width not divisible by 32 were distorted. 854x480

video distorted 854 / 32 = 26.22

640x480

video good 640 / 32 = 20.0

618x480

video distorted 618/32 = 19.10

960x480

video good 960/32 = 30.0

420x480

video distorted 420/32 = 13.4

360x480

video good 360/32 = 11.8

The last example is another wrinkle in this problem. The width (360) is not divisible by 32. I did some further digging into this and found that the videos Pixel Format also influences if the video is distorted. Attract is converting all videos to AV_PIX_FMT_RGBA. I added more debug logging to identify the pixel format. I found that my videos are in two pixel formats (videos scrapped from screenscraper.fr):

AV_PIX_FMT_YUV420P AV_PIX_FMT_YUV444P

I created two rom lists with 10+ entries:

List of AV_PIX_FMT_YUV420P videos with width not divisible by 32 List of AV_PIX_FMT_YUV444P videos with width not divisible by 32

Further testing with Valgrind and I found that AV_PIX_FMT_YUV444P videos don't have invalid writes when align=1 Some AV_PIX_FMT_YUV420P videos display ok with align=32. Some videos only display with align=1, but Valgrind reports invalid writes. Also it's very easy to crash attract with align=1. Current logic I'm using that appears to be stable and makes the most videos visible: Default: align=32 If (width % 32 > 0) If (px_fmt == AV_PIX_FMT_YUV444P) Then align=1 If(px_fmt == AV_PIX_FMT_YUV420P) Then align=32

I've tried AV_PIX_FMT_YUV420P with align: 1, 2, 3, 4, 8, 16. Fixes some videos, but some still cause a core dump. Also tried keeping AV_PIX_FMT_YUV420P as AV_PIX_FMT_YUV420P or converting to AV_PIX_FMT_YUV444P. No crashes, but video is still distorted. Seems attract needs AV_PIX_FMT_RGBA. (AV_PIX_FMT_RGB32 + align=32) works for some, but not for the same videos that have problem with (AV_PIX_FMT_RGBA + align=32). All my tests were done with attractplus. I assume the same issue is happening with attract.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

spilinek commented 1 year ago

Your change in https://github.com/mickelson/attract/commit/51d3fefbeeab5dfd5a9dbafce78e25f352e4fe9c fixed it for attract and attractplus. No more crashes and no more distorted videos. Thanks!