opus stream chaining #205

cathugger · 2018-01-26T15:48:38Z

support for chaining streams to enable changing stream' metadata on the fly.
currently support is opt-in because lots of clients can't handle this properly yet.

MaxKellermann · 2018-01-26T15:52:25Z

Without looking at the code: if your PR contains text, this means that your commit message is bad.

cathugger · 2018-01-26T17:05:06Z

should I change it?

MaxKellermann · 2018-02-16T19:56:35Z

Please fold the two commits (the second one is just fixup for the first one, even though it's not documented that way) and add a proper commit message.
Your first patch is hard to read because you moved original code around and renamed a method; that appears in the diff, even though nothing (but the method name) was changed. Please move that back so I can read your patch properly.

cathugger · 2018-02-16T23:13:07Z

@MaxKellermann done. it's hard to read because back when I changed it, it was kind of rewrite.
there are some changes on how it inserts tags into stream (old version overrides Read() to inject them, my version pushes them when tags appear because otherwise I'd need to hold tags somewhere waiting for Read() and doing that didn't make much sense to me).
also some unrelated changes like changing mime type from "audio/ogg" to "audio/ogg; codecs=opus" because that made more sense to me (I can get rid of this one if you want, I didn't observed any changes in clients from it).

cathugger · 2018-02-16T23:36:44Z

@MaxKellermann I attempted to make it easier to review by moving functions to be closer to their original locations

MaxKellermann · 2018-02-16T23:38:57Z

src/encoder/plugins/OpusEncoderPlugin.cxx

@@ -183,24 +190,25 @@ OpusEncoder::~OpusEncoder()
 void
 OpusEncoder::DoEncode(bool eos)
 {
-	assert(buffer_position == buffer_size);
+	if (!eos)
+		assert(buffer_position == buffer_size);


You shouldn't call a macro after an if statement without curly braces, because this is unsafe; and it doesn't make sense to check the eos flag in the non-debug build. It's better to move that check into the assert().

MaxKellermann · 2018-02-16T23:40:40Z

src/encoder/plugins/OpusEncoderPlugin.cxx

@@ -84,7 +88,7 @@ class PreparedOpusEncoder final : public PreparedEncoder {
 	Encoder *Open(AudioFormat &audio_format) override;

 	const char *GetMimeType() const override {
-		return "audio/ogg";
+		return "audio/ogg; codecs=opus";


This change has nothing to do with chaining feature, does it? Should be a separate commit; I can cherry-pick such a tiny commit easily, even if there are issues left with the "large" commit.

it has nothing to do with chaining feature, yes

From RFC 7845:
An "Ogg Opus file" consists of one or more sequentially multiplexed
segments, each containing exactly one Ogg Opus stream. The
RECOMMENDED mime-type for Ogg Opus files is "audio/ogg".

If more specificity is desired, one MAY indicate the presence of Opus
streams using the codecs parameter defined in [RFC6381] and
[RFC5334], e.g.,

audio/ogg; codecs=opus

for an Ogg Opus file.

so I guess I should leave it as audio/ogg, as it's recommended

MaxKellermann · 2018-02-16T23:41:46Z

src/encoder/plugins/OpusEncoderPlugin.cxx

 };

 class PreparedOpusEncoder final : public PreparedEncoder {
 	opus_int32 bitrate;
 	int complexity;
 	int signal;
+	bool chaining;


Make this const and use a C++ initializer in the constructor.

rest of things are parsed and initialized inside PreparedEncoder::PreparedEncoder()'s body.
I just did things in consistent way. Should I really change this?

though they all had reason to be in body, because they couldn't be done with just initializer so I guess I will

That's because the other attributes have more complex parsers. But I welcome patches which move the parsers into separate (throwing) functions and make the other attributes const as well. The old code was from a time when MPD had C++ exceptions disabled (-fno-exceptions) and error handling was different (more complicated).

MaxKellermann · 2018-02-16T23:42:23Z

src/encoder/plugins/OpusEncoderPlugin.cxx

 	if (result < 0)
 		throw std::runtime_error("Opus encoder error");

-	granulepos += buffer_frames;
+	granulepos += buffer_position / frame_size;


What is this change about?

on final stream packet (one with eos set), we are allowed to have granulepos less than packet actually contains. decoder should take smaller granulepos into account and discard unused data. this is for stream cutting - we need to push final packet to inject tags, but we don't have enough data for whole packet. therefore we encode full packet, but specify lesser granulepos.

MaxKellermann · 2018-02-16T23:43:02Z

src/encoder/plugins/OpusEncoderPlugin.cxx

@@ -52,30 +53,33 @@ class OpusEncoder final : public OggEncoder {

 	ogg_int64_t packetno = 0;

-	ogg_int64_t granulepos;
+	ogg_int64_t granulepos = 0;


Why does this need to be initialized now? Was this a bug previously, or did your other code changes require it?

Was this a bug previously

probably. it's not initialised anywhere else. technically granulepos in stream can start from any value, so decoders probably didn't complain and it was unnoticed.

You're right, so this change has nothing to do with this patch. Separate patch, please, so I can cherry-pick this into the stable branch v0.20.x

@MaxKellermann #222

MaxKellermann · 2018-02-16T23:47:27Z

Thanks, much better now. And each time before you push, please rebase on my master branch.

cathugger · 2018-02-17T00:10:20Z

@MaxKellermann did changes you requested. didn't fold yet to not disturb previous review

cathugger · 2018-02-17T01:08:20Z

squashed

MaxKellermann · 2018-02-25T08:47:36Z

Thanks, this looks good now. Now what's missing is a patch for doc/user.xml with documentation and maybe a NEWS line telling people about it, then I'll merge it.

cathugger · 2018-02-25T14:32:41Z

@MaxKellermann documented.
hopefully I got docs syntax right.
NEWS line isn't exactly same as commit message, I hope that's ok.

cathugger · 2018-02-25T14:44:27Z

uh, documentation isn't quite correct, it uses ICY headers only in case of http output plugin. need to fix

cathugger · 2018-02-25T15:00:16Z

fixed documentation. now should be all good.

support for chaining ogg opus streams to enable changing stream' metadata on the fly. currently support is opt-in (enabled by additional option) because lots of clients can't handle this properly yet.

cathugger · 2018-02-25T15:27:30Z

pushed some minor syntax fixes

cathugger mentioned this pull request Jan 26, 2018

Empty metadata when streaming to icecast with opus encoder #70

Closed

MaxKellermann requested changes Feb 16, 2018

View reviewed changes

encoder/opus: add optional stream chaining support

47d1d3c

support for chaining ogg opus streams to enable changing stream' metadata on the fly. currently support is opt-in (enabled by additional option) because lots of clients can't handle this properly yet.

MaxKellermann merged commit 47d1d3c into MusicPlayerDaemon:master Feb 26, 2018

cathugger deleted the opustags-pr branch February 26, 2018 21:59

JustArchi mentioned this pull request Aug 22, 2018

opustags issues (stream finish at song change, duplicated metadata) #344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opus stream chaining #205

opus stream chaining #205

cathugger commented Jan 26, 2018

MaxKellermann commented Jan 26, 2018

cathugger commented Jan 26, 2018

MaxKellermann commented Feb 16, 2018

cathugger commented Feb 16, 2018

cathugger commented Feb 16, 2018

MaxKellermann Feb 16, 2018

cathugger Feb 16, 2018

MaxKellermann Feb 16, 2018

cathugger Feb 16, 2018

cathugger Feb 17, 2018

MaxKellermann Feb 16, 2018

cathugger Feb 17, 2018

cathugger Feb 17, 2018

MaxKellermann Feb 17, 2018

MaxKellermann Feb 16, 2018

cathugger Feb 16, 2018

MaxKellermann Feb 16, 2018

cathugger Feb 16, 2018 •

edited

MaxKellermann Feb 17, 2018

cathugger Feb 17, 2018

MaxKellermann commented Feb 16, 2018

cathugger commented Feb 17, 2018

cathugger commented Feb 17, 2018

MaxKellermann commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

opus stream chaining #205

opus stream chaining #205

Conversation

cathugger commented Jan 26, 2018

MaxKellermann commented Jan 26, 2018

cathugger commented Jan 26, 2018

MaxKellermann commented Feb 16, 2018

cathugger commented Feb 16, 2018

cathugger commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathugger Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKellermann commented Feb 16, 2018

cathugger commented Feb 17, 2018

cathugger commented Feb 17, 2018

MaxKellermann commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger commented Feb 25, 2018

cathugger Feb 16, 2018 •

edited