diff --git a/NEWS b/NEWS index bc914601c..bee9f47ec 100644 --- a/NEWS +++ b/NEWS @@ -5,9 +5,14 @@ ver 0.22 (not yet released) ver 0.21.6 (not yet released) * protocol - allow loading playlists specified as absolute filesystem paths + - fix negated filter expressions with multiple tag values - fix "list" with filter expression * input - cdio_paranoia: fix build failure due to missing #include +* decoder + - opus: fix replay gain when there are no other tags +* output + - pulse: work around error with unusual channel count * playlist - flac: fix use-after-free bug * support abstract sockets on Linux diff --git a/doc/protocol.rst b/doc/protocol.rst index 41bb2a861..bbe1a655b 100644 --- a/doc/protocol.rst +++ b/doc/protocol.rst @@ -144,15 +144,20 @@ syntax:: ``EXPRESSION`` is a string enclosed in parantheses which can be one of: -- ``(TAG == 'VALUE')``: match a tag value. - ``(TAG != 'VALUE')``: mismatch a tag value. - The special tag "*any*" checks all - tag values. - *albumartist* looks for +- ``(TAG == 'VALUE')``: match a tag value; if there are multiple + values of the given type, at least one must match. + ``(TAG != 'VALUE')``: mismatch a tag value; if there are multiple + values of the given type, none of them must match. + The special tag ``any`` checks all + tag types. + ``AlbumArtist`` looks for ``VALUE`` in ``AlbumArtist`` and falls back to ``Artist`` tags if ``AlbumArtist`` does not exist. ``VALUE`` is what to find. + An empty value string means: match only if the given tag type does + not exist at all; this implies that negation with an empty value + checks for the existence of the given tag type. - ``(TAG contains 'VALUE')`` checks if the given value is a substring of the tag value. @@ -178,7 +183,7 @@ of: - ``(AudioFormat =~ 'SAMPLERATE:BITS:CHANNELS')``: matches the audio format with the given mask (i.e. one - or more attributes may be "*"). + or more attributes may be ``*``). - ``(!EXPRESSION)``: negate an expression. Note that each expression must be enclosed in parantheses, e.g. :code:`(!(artist == 'VALUE'))` @@ -207,11 +212,11 @@ backslash. Example expression which matches an artist named ``foo'bar"``:: - (artist "foo\'bar\"") + (Artist == "foo\'bar\"") At the protocol level, the command must look like this:: - find "(artist \"foo\\'bar\\\"\")" + find "(Artist == \"foo\\'bar\\\"\")" The double quotes enclosing the artist name must be escaped because they are inside a double-quoted ``find`` parameter. The single quote diff --git a/src/decoder/plugins/OpusDecoderPlugin.cxx b/src/decoder/plugins/OpusDecoderPlugin.cxx index 542f71540..3b3c312af 100644 --- a/src/decoder/plugins/OpusDecoderPlugin.cxx +++ b/src/decoder/plugins/OpusDecoderPlugin.cxx @@ -208,10 +208,12 @@ MPDOpusDecoder::HandleTags(const ogg_packet &packet) TagBuilder tag_builder; AddTagHandler h(tag_builder); - if (ScanOpusTags(packet.packet, packet.bytes, &rgi, h) && - !tag_builder.empty()) { - client.SubmitReplayGain(&rgi); + if (!ScanOpusTags(packet.packet, packet.bytes, &rgi, h)) + return; + client.SubmitReplayGain(&rgi); + + if (!tag_builder.empty()) { Tag tag = tag_builder.Commit(); auto cmd = client.SubmitTag(input_stream, std::move(tag)); if (cmd != DecoderCommand::NONE) diff --git a/src/output/plugins/PulseOutputPlugin.cxx b/src/output/plugins/PulseOutputPlugin.cxx index 59fe9fb54..fa1b06da9 100644 --- a/src/output/plugins/PulseOutputPlugin.cxx +++ b/src/output/plugins/PulseOutputPlugin.cxx @@ -581,8 +581,8 @@ PulseOutput::SetupStream(const pa_sample_spec &ss) /* WAVE-EX is been adopted as the speaker map for most media files */ pa_channel_map chan_map; - pa_channel_map_init_auto(&chan_map, ss.channels, - PA_CHANNEL_MAP_WAVEEX); + pa_channel_map_init_extend(&chan_map, ss.channels, + PA_CHANNEL_MAP_WAVEEX); stream = pa_stream_new(context, name, &ss, &chan_map); if (stream == nullptr) throw MakePulseError(context, diff --git a/src/song/StringFilter.cxx b/src/song/StringFilter.cxx index 2f9c26039..4a72f8216 100644 --- a/src/song/StringFilter.cxx +++ b/src/song/StringFilter.cxx @@ -22,13 +22,10 @@ #include -inline bool +bool StringFilter::MatchWithoutNegation(const char *s) const noexcept { -#if !CLANG_CHECK_VERSION(3,6) - /* disabled on clang due to -Wtautological-pointer-compare */ assert(s != nullptr); -#endif #ifdef HAVE_PCRE if (regex) diff --git a/src/song/StringFilter.hxx b/src/song/StringFilter.hxx index a1a281f3c..588e34bdb 100644 --- a/src/song/StringFilter.hxx +++ b/src/song/StringFilter.hxx @@ -105,7 +105,9 @@ public: gcc_pure bool Match(const char *s) const noexcept; -private: + /** + * Like Match(), but ignore the "negated" flag. + */ gcc_pure bool MatchWithoutNegation(const char *s) const noexcept; }; diff --git a/src/song/TagSongFilter.cxx b/src/song/TagSongFilter.cxx index d7169740c..00f2021aa 100644 --- a/src/song/TagSongFilter.cxx +++ b/src/song/TagSongFilter.cxx @@ -35,42 +35,41 @@ TagSongFilter::ToExpression() const noexcept } bool -TagSongFilter::MatchNN(const TagItem &item) const noexcept -{ - return (type == TAG_NUM_OF_ITEM_TYPES || item.type == type) && - filter.Match(item.value); -} - -bool -TagSongFilter::MatchNN(const Tag &tag) const noexcept +TagSongFilter::Match(const Tag &tag) const noexcept { bool visited_types[TAG_NUM_OF_ITEM_TYPES]{}; for (const auto &i : tag) { visited_types[i.type] = true; - if (MatchNN(i)) - return true; + if ((type == TAG_NUM_OF_ITEM_TYPES || i.type == type) && + filter.MatchWithoutNegation(i.value)) + return !filter.IsNegated(); } if (type < TAG_NUM_OF_ITEM_TYPES && !visited_types[type]) { + /* if the specified tag is not present, try the + fallback tags */ + bool result = false; - if (ApplyTagFallback(type, - [&](TagType tag2) { - if (!visited_types[tag2]) - return false; + if (ApplyTagFallback(type, [&](TagType tag2) { + if (!visited_types[tag2]) + /* we already know that this tag type + isn't present, so let's bail out + without checking again */ + return false; - for (const auto &item : tag) { - if (item.type == tag2 && - filter.Match(item.value)) { - result = true; - break; - } - } + for (const auto &item : tag) { + if (item.type == tag2 && + filter.MatchWithoutNegation(item.value)) { + result = true; + break; + } + } - return true; - })) - return result; + return true; + })) + return result != filter.IsNegated(); /* If the search critieron was not visited during the sweep through the song's tag, it means this field @@ -79,14 +78,14 @@ TagSongFilter::MatchNN(const Tag &tag) const noexcept then it's a match as well and we should return true. */ if (filter.empty()) - return true; + return !filter.IsNegated(); } - return false; + return filter.IsNegated(); } bool TagSongFilter::Match(const LightSong &song) const noexcept { - return MatchNN(song.tag); + return Match(song.tag); } diff --git a/src/song/TagSongFilter.hxx b/src/song/TagSongFilter.hxx index e60910934..3f9f7051b 100644 --- a/src/song/TagSongFilter.hxx +++ b/src/song/TagSongFilter.hxx @@ -68,8 +68,7 @@ public: bool Match(const LightSong &song) const noexcept override; private: - bool MatchNN(const Tag &tag) const noexcept; - bool MatchNN(const TagItem &tag) const noexcept; + bool Match(const Tag &tag) const noexcept; }; #endif diff --git a/src/tag/Fallback.hxx b/src/tag/Fallback.hxx index 08e4dc80a..ecbe0d439 100644 --- a/src/tag/Fallback.hxx +++ b/src/tag/Fallback.hxx @@ -22,6 +22,11 @@ #include +/** + * Invoke the given function for all fallback tags of the given + * #TagType, until the function returns true (or until there are no + * more fallback tags). + */ template bool ApplyTagFallback(TagType type, F &&f) noexcept @@ -43,6 +48,11 @@ ApplyTagFallback(TagType type, F &&f) noexcept return false; } +/** + * Invoke the given function for the given #TagType and all of its + * fallback tags, until the function returns true (or until there are + * no more fallback tags). + */ template bool ApplyTagWithFallback(TagType type, F &&f) noexcept diff --git a/test/MakeTag.hxx b/test/MakeTag.hxx new file mode 100644 index 000000000..49e221b35 --- /dev/null +++ b/test/MakeTag.hxx @@ -0,0 +1,45 @@ +/* + * Copyright 2003-2019 The Music Player Daemon Project + * http://www.musicpd.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "tag/Builder.hxx" +#include "tag/Tag.hxx" +#include "util/Compiler.h" + +inline void +BuildTag(gcc_unused TagBuilder &tag) noexcept +{ +} + +template +inline void +BuildTag(TagBuilder &tag, TagType type, const char *value, + Args&&... args) noexcept +{ + tag.AddItem(type, value); + BuildTag(tag, std::forward(args)...); +} + +template +inline Tag +MakeTag(Args&&... args) noexcept +{ + TagBuilder tag; + BuildTag(tag, std::forward(args)...); + return tag.Commit(); +} diff --git a/test/TestTagSongFilter.cxx b/test/TestTagSongFilter.cxx new file mode 100644 index 000000000..230409631 --- /dev/null +++ b/test/TestTagSongFilter.cxx @@ -0,0 +1,163 @@ +/* + * Copyright 2003-2019 The Music Player Daemon Project + * http://www.musicpd.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "MakeTag.hxx" +#include "song/TagSongFilter.hxx" +#include "song/LightSong.hxx" +#include "tag/Type.h" + +#include + +static bool +InvokeFilter(const TagSongFilter &f, const Tag &tag) noexcept +{ + return f.Match(LightSong("dummy", tag)); +} + +TEST(TagSongFilter, Basic) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle", TAG_TITLE, "foo"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo", TAG_TITLE, "needle", TAG_ALBUM, "bar"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_ARTIST, "needle"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedleBAR"))); +} + +/** + * Test with empty string. This matches tags where the given tag type + * does not exist. + */ +TEST(TagSongFilter, Empty) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); +} + +TEST(TagSongFilter, Substring) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, true, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "needleBAR"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "FOOneedleBAR"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "eedle"))); +} + +TEST(TagSongFilter, Negated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); +} + +/** + * Combine the "Empty" and "Negated" tests. + */ +TEST(TagSongFilter, EmptyNegated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("", false, false, true)); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo"))); +} + +/** + * Negation with multiple tag values. + */ +TEST(TagSongFilter, MultiNegated) +{ + const TagSongFilter f(TAG_TITLE, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "needle", TAG_TITLE, "bar"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_TITLE, "foo", TAG_TITLE, "needle"))); +} + +/** + * Check whether fallback tags work, e.g. AlbumArtist falls back to + * just Artist if there is no AlbumArtist. + */ +TEST(TagSongFilter, Fallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("needle", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle"))); + + EXPECT_FALSE(InvokeFilter(f, MakeTag())); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); + + /* no fallback, thus the Artist tag isn't used and this must + be a mismatch */ + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle", TAG_ALBUM_ARTIST, "foo"))); +} + +/** + * Combine the "Empty" and "Fallback" tests. + */ +TEST(TagSongFilter, EmptyFallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("", false, false, false)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); +} + +/** + * Combine the "Negated" and "Fallback" tests. + */ +TEST(TagSongFilter, NegatedFallback) +{ + const TagSongFilter f(TAG_ALBUM_ARTIST, + StringFilter("needle", false, false, true)); + + EXPECT_TRUE(InvokeFilter(f, MakeTag())); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ALBUM_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "foo"))); + EXPECT_FALSE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle"))); + EXPECT_TRUE(InvokeFilter(f, MakeTag(TAG_ARTIST, "needle", TAG_ALBUM_ARTIST, "foo"))); +} diff --git a/test/meson.build b/test/meson.build index 628e5cbfd..7345283d9 100644 --- a/test/meson.build +++ b/test/meson.build @@ -20,16 +20,6 @@ gtest_dep = declare_dependency( subdir('net') -executable( - 'ParseSongFilter', - 'ParseSongFilter.cxx', - include_directories: inc, - dependencies: [ - song_dep, - pcm_dep, - ], -) - executable( 'read_conf', 'read_conf.cxx', @@ -211,6 +201,33 @@ if zlib_dep.found() ) endif +# +# Filter +# + +executable( + 'ParseSongFilter', + 'ParseSongFilter.cxx', + include_directories: inc, + dependencies: [ + song_dep, + pcm_dep, + ], +) + +test( + 'TestSongFilter', + executable( + 'TestSongFilter', + 'TestTagSongFilter.cxx', + include_directories: inc, + dependencies: [ + song_dep, + gtest_dep, + ], + ) +) + # # Neighbor # diff --git a/test/test_translate_song.cxx b/test/test_translate_song.cxx index 2419b1bac..cd4e39a5c 100644 --- a/test/test_translate_song.cxx +++ b/test/test_translate_song.cxx @@ -2,6 +2,7 @@ * Unit tests for playlist_check_translate_song(). */ +#include "MakeTag.hxx" #include "playlist/PlaylistSong.hxx" #include "song/DetachedSong.hxx" #include "SongLoader.hxx" @@ -38,28 +39,6 @@ uri_supported_scheme(const char *uri) noexcept static constexpr auto music_directory = PATH_LITERAL("/music"); static Storage *storage; -static void -BuildTag(gcc_unused TagBuilder &tag) -{ -} - -template -static void -BuildTag(TagBuilder &tag, TagType type, const char *value, Args&&... args) -{ - tag.AddItem(type, value); - BuildTag(tag, std::forward(args)...); -} - -template -static Tag -MakeTag(Args&&... args) -{ - TagBuilder tag; - BuildTag(tag, std::forward(args)...); - return tag.Commit(); -} - static Tag MakeTag1a() {