search

DeaDBeeF-Player / deadbeef

DeaDBeeF Player
https://deadbeef.sourceforge.io/
Other
1.61k stars 176 forks source link

Autosort is broken #3127

Open rsekman opened 3 days ago

rsekman commented 3 days ago

Steps to reproduce the problem

Create a playlist and add some files. Enable autosort. Sort the playlist. Add more files.

What's going on? Describe the problem in as much detail as possible.

Expected behaviour: the playlist is sorted. Actual behaviour: the files are added to the end of the playlist regardless of where they should be. Sorting manually works correctly.

I wrote tests (see linked PR) and stepped through the code with gdb. The cause appears to be that in plt_sort_internal, the title formatting string to use is reset: https://github.com/DeaDBeeF-Player/deadbeef/blob/2a5b3cf2d083fea7d42c389403b9e0b71257a842/src/sort.c#L236-L239 When plt_sort_internal is called from plt_autosort the metadata value is set to the empty string even if format is non-empty. This does not happen when calling plt_sort_v2 directly, even though the arguments are the same.

My tests look like

const char sort_tf[] = "%artist%";
playlist_t *plt = plt_alloc("test");

//insert some items out of order
// ...

// direct sorting test:
plt_sort_v2(plt, PL_MAIN, -1, sort_tf, DDB_SORT_ASCENDING);
EXPECT_STREQ(plt_find_meta(plt, "autosort_tf"), sort_tf);
// test that the items are in order...

// autosorting test:
plt_autosort(plt);
EXPECT_STREQ(plt_find_meta(plt, "autosort_tf"), sort_tf);
// test that the items are in order...

The first test passes but the second succeeds. The output from running them in the debbuger with a breakpoint in plt_sort_v2 looks like this

[ RUN      ] PlaylistTests.test_PlaylistSort

Thread 1 "runtests" hit Breakpoint 1, plt_sort_v2 (plt=0x55555575d0b0, iter=0, id=-1, format=0x7fffffffc530 "%artist%", order=1)
    at src/sort.c:46
46     ddb_undobuffer_t *undobuffer = ddb_undomanager_get_buffer (ddb_undomanager_shared ());
(gdb) continue
Continuing.
[       OK ] PlaylistTests.test_PlaylistSort (68796 ms)
[ RUN      ] PlaylistTests.test_PlaylistAutosort

Thread 1 "runtests" hit Breakpoint 1, plt_sort_v2 (plt=0x55555575d0b0, iter=0, id=-1, format=0x55555575d335 "%artist%", order=1)
    at src/sort.c:46
46     ddb_undobuffer_t *undobuffer = ddb_undomanager_get_buffer (ddb_undomanager_shared ());
(gdb) bt
#0  plt_sort_v2 (plt=0x55555575d0b0, iter=0, id=-1, format=0x55555575d335 "%artist%", order=1) at src/sort.c:46
#1  0x0000555555622464 in plt_autosort (plt=0x55555575d0b0) at src/sort.c:412
#2  0x00005555556815c7 in PlaylistTests_test_PlaylistAutosort_Test::TestBody (this=<optimized out>) at Tests/PlaylistTests.cpp:117
#3  0x0000555555674dab in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>
    (method=&virtual testing::Test::TestBody(), location=0x55555559c0a6 "the test body", object=<optimized out>)
    at external/googletest/googletest/src/gtest.cc:2621
#4  testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>
    (object=object@entry=0x555555765160, method=&virtual testing::Test::TestBody(), location=0x55555559c0a6 "the test body")
    at external/googletest/googletest/src/gtest.cc:2657
#5  0x0000555555658460 in testing::Test::Run (this=this@entry=0x555555765160) at external/googletest/googletest/src/gtest.cc:2696
#6  0x000055555565953c in testing::TestInfo::Run (this=0x555555714260) at external/googletest/googletest/src/gtest.cc:2845
#7  0x000055555565a055 in testing::TestSuite::Run (this=0x555555713d70) at external/googletest/googletest/src/gtest.cc:3004
#8  0x000055555566b69e in testing::internal::UnitTestImpl::RunAllTests (this=0x555555713a80) at external/googletest/googletest/src/gtest.cc:5889
#9  0x0000555555675c5b in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
    (method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x55555566b280 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x555555597587 "auxiliary test code (environments or event listeners)", object=<optimized out>)
    at external/googletest/googletest/src/gtest.cc:2621
#10 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
    (object=<optimized out>, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x55555566b280 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x555555597587 "auxiliary test code (environments or event listeners)")
    at external/googletest/googletest/src/gtest.cc:2657
#11 0x000055555566b223 in testing::UnitTest::Run (this=0x5555556efed0 <testing::UnitTest::GetInstance()::instance>)
    at external/googletest/googletest/src/gtest.cc:5454
#12 0x0000555555680b4f in RUN_ALL_TESTS () at external/googletest/googletest/include/gtest/gtest.h:2310
#13 main (argc=1, argv=0x7fffffffd8f8) at Tests/gtest-runner.cpp:30
(gdb) continue
Continuing.
Tests/PlaylistTests.cpp:119: Failure
Expected equality of these values:
  plt_find_meta(plt, "autosort_tf")
    Which is: ""
  sort_tf
    Which is: "%artist%"
Tests/PlaylistTests.cpp:126: Failure
Expected equality of these values:
  "C"
  pl_find_meta_raw(tail, "artist")
    Which is: "B"
[  FAILED  ] PlaylistTests.test_PlaylistAutosort (20658 ms)

You can see that the calls are the same except for direct sorting the tf string is on the stack and for autosorting it is on the heap. That is because it was retrieved from the cache in the latter case. It seems that there is a problem with cache invalidation. I don't understand how this can be because everything is passed as const in this call stack.

Information about the software:

Deadbeef version: built from master OS: Arch linux

rsekman commented 3 days ago

None of the code here seems to have been touched recently (years), so I also don't understand when this happened, because I distinctly remember autosort working not too long ago.