Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Airplay cover art to metadata #543

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Add Airplay cover art to metadata #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62fe9c9
Add support for cover art
cmfcmf Jan 29, 2020
9561ae7
Fix 'Bad Address' error when sending large messages
cmfcmf Jan 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions server/control_session_tcp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,20 @@ void ControlSessionTcp::sendAsync(const std::string& message)

void ControlSessionTcp::send_next()
{
auto message = messages_.front();
boost::asio::async_write(socket_, boost::asio::buffer(message + "\r\n"),
// We cannot simply do boost::asio::buffer(message + "\r\n"), because 'message + "\r\n"' becomes invalid
// after leaving this method, leading to a 'Bad address' error.
// Instead, we need to mutate the string before passing it to boost::asio::buffer.
//
// From the boost reference:
//
// A buffer object does not have any ownership of the memory it refers to. It is the responsibility of
// the application to ensure the memory region remains valid until it is no longer required for an I/O
// operation. When the memory is no longer available, the buffer is said to have been invalidated.
// https://www.boost.org/doc/libs/1_72_0/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.buffer_invalidation

string& message = messages_.front();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bug is a nasty one, great that you found it. But it's not related to the metadata, right? It looks like meta data is sent to the streaming clients, and not to the control clients.
Also I think, I would prefer adding the \r\n when adding the message to the queue:

void ControlSessionTcp::sendAsync(const std::string& message)
{
    strand_.post([ this, self = shared_from_this(), message ]() {
        messages_.emplace_back(message + "\r\n");

Copy link
Contributor Author

@cmfcmf cmfcmf Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for merging!

While the code may not be related to the metadata handling, the issue only surfaced when I sent big metadata cover images. I assume that the the big metadata strings resulted in a different/more rapidly changing/... stack/heap, causing the undefined behavior created by this bug to actually do something bad (crash the control server).

message += "\r\n";
boost::asio::async_write(socket_, boost::asio::buffer(message),
boost::asio::bind_executor(strand_, [ this, self = shared_from_this() ](std::error_code ec, std::size_t length) {
messages_.pop_front();
if (ec)
Expand Down
98 changes: 85 additions & 13 deletions server/streamreader/airplay_stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,19 @@ AirplayStream::AirplayStream(PcmListener* pcmListener, boost::asio::io_context&
logStderr_ = true;

string devicename = uri_.getQuery("devicename", "Snapcast");
params_wo_port_ = "\"--name=" + devicename + "\" --output=stdout --use-stderr";
params_wo_port_ = "\"--name=" + devicename + "\" --output=stdout --use-stderr --get-coverart";

port_ = cpt::stoul(uri_.getQuery("port", "5000"));
setParamsAndPipePathFromPort();

#ifdef HAS_EXPAT
createParser();
metadata_dirty_ = false;
metadata_ = json();
metadata_["ALBUM"] = "";
metadata_["ARTIST"] = "";
metadata_["TITLE"] = "";
metadata_["COVER"] = "";
#else
LOG(INFO, LOG_TAG) << "Metadata support not enabled (HAS_EXPAT not defined)" << "\n";
#endif
Expand Down Expand Up @@ -99,24 +105,90 @@ void AirplayStream::createParser()

void AirplayStream::push()
{
// The metadata we collect consists of two parts:
// (1) ALBUM, ARTIST, TITLE
// (2) COVER
//
// This stems from the Airplay protocol, which treats cover art differently from the rest of the metadata.
//
// The process for (1) is as follows:
// - The ssnc->mdst message is sent ("metadata start")
// - core->asal|asar|minm messages are sent
// - The ssnc->mden message is sent ("metadata end")
// This process can repeat multiple times *for the same song*, with *the same metadata*.
//
// The process for (2) is as follows:
// - The ssnc->pcst message is sent ("picture start")
// - The ssnc->PICT message is sent (picture contents)
// - The ssnc->pcen message is sent ("picture end")
// If no cover art is available, the PICT message's data has a length of 0 *or* none of the messages are sent.
//
// Here is an example from an older iPad:
//
// User plays song without cover art
// - empty cover art message (2)
// - empty cover art message (2)
// - metadata message (1)
// - metadata message (1)
// - metadata message (1)
// User selects next song without cover art
// - metadata message (1)
// - metadata message (1)
// User selects next song with cover art
// - metadata message (1)
// - metadata message (1)
// - cover art message (2)
// - metadata message (1)
// User selects next song with cover art
// - metadata message (1)
// - metadata message (1)
// - empty cover art message (2) (!)
// - metadata message (1)
// - cover art message (2)
//
// As can be seen, the order of metadata (1) and cover (2) messages is non-deterministic.
// That is why we call setMeta() on both end of message (1) and (2).
string data = entry_->data;
if (entry_->isBase64 && entry_->length > 0)

// Do not base64 decode cover art
const bool is_cover = entry_->type == "ssnc" && entry_->code == "PICT";
if (!is_cover && entry_->isBase64 && entry_->length > 0)
data = base64_decode(data);

if (entry_->type == "ssnc" && entry_->code == "mdst")
jtag_ = json();
if (is_cover)
{
setMetaData("COVER", data);
// LOG(INFO, LOG_TAG) << "Metadata type: " << entry_->type << " code: " << entry_->code << " data length: " << data.length() << "\n";
}
else
{
// LOG(INFO, LOG_TAG) << "Metadata type: " << entry_->type << " code: " << entry_->code << " data: " << data << "\n";
}

if (entry_->code == "asal")
jtag_["ALBUM"] = data;
if (entry_->code == "asar")
jtag_["ARTIST"] = data;
if (entry_->code == "minm")
jtag_["TITLE"] = data;
if (entry_->type == "core" && entry_->code == "asal")
setMetaData("ALBUM", data);
if (entry_->type == "core" && entry_->code == "asar")
setMetaData("ARTIST", data);
if (entry_->type == "core" && entry_->code == "minm")
setMetaData("TITLE", data);

if (entry_->type == "ssnc" && entry_->code == "mden")
// mden = metadata end, pcen == picture end
if (metadata_dirty_ && entry_->type == "ssnc" && (entry_->code == "mden" || entry_->code == "pcen"))
{
setMeta(metadata_);
metadata_dirty_ = false;
}
}

void AirplayStream::setMetaData(const string& key, const string& newValue)
{
// Only overwrite metadata and set metadata_dirty_ if the metadata has changed.
// This avoids multiple unnecessary transmissions of the same metadata.
const auto& oldValue = metadata_[key];
if (oldValue != newValue)
{
// LOG(INFO, LOG_TAG) << "metadata=" << jtag_.dump(4) << "\n";
setMeta(jtag_);
metadata_[key] = newValue;
metadata_dirty_ = true;
}
}
#endif
Expand Down
5 changes: 4 additions & 1 deletion server/streamreader/airplay_stream.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,17 @@ class AirplayStream : public ProcessStream
XML_Parser parser_;
std::unique_ptr<TageEntry> entry_;
std::string buf_;
json jtag_;
json metadata_;
// set whenever metadata_ has changed
bool metadata_dirty_;
#endif

void pipeReadLine();
#ifdef HAS_EXPAT
int parse(std::string line);
void createParser();
void push();
void setMetaData(const std::string&, const std::string&);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add parameter names

#endif

void setParamsAndPipePathFromPort();
Expand Down