Allow pipeline writer to spit out Ogg directly, including when seeking

[philippe44]

Use ogg v 0.8 to use directly inner writer instead of the complicated workaround in previous patch #560

[sashahilton00]

I've merged a fix for the CI, and updated the Cargo.lock file, along with bumping the minimum rust version to 1.46.0, since the current rust version is 1.49.0, and vec_drain_as_slice was introduced in 1.46.0 and thus was causing issues during compilation.

[philippe44]

Thanks!

[ashthespy]

along with bumping the minimum rust version to 1.46.0, since the current rust version is 1.49.0, and vec_drain_as_slice was introduced in 1.46.0 and thus was causing issues during compilation.

Shouldn't this then be a feature flag gated feature?
For the core library, it would be nice to maintain Debian's current stable (buster) version of Rust as the MSRV (1.41).

[sashahilton00]

@ashthespy probably not a bad idea. I personally don't care about keeping the supported compiler version low, but I can see the argument for it. @philippe44 are you ok to add in a compile time flag for this feature and drop the CI MSRV back to 1.41.0?

[philippe44]

I'll have to look at how to make that a clean compile time option (I'm a Rust noob, so everything requires learning) or I'll see if I can use another way to get the data instead of using vec_drain_as_slice. This is that code

                 info!("ending packet of one byte");
                        let last = data.drain(..);
                        return Ok(Some(AudioPacket(vec![i16::from(last.as_slice()[0])])));

and all I need is to take the last byte and create an i16 Vec out of that...

[plietar]

Isn’t that code just equivalent to vec![data[0] as i16]?

[plietar]

I kind of worry about passing the OGG data as i16s. For instance in this particular line, an extraneous byte gets inserted in the u8 -> i16 -> u8 conversion process.

I think it would be more appropriate to turn AudioPacket into a enum with a Vec entry for PCM samples and a Vec for OGG data.

The pipe backend matches on the packet type and does the right thing. Other backends crash on OGG data.

[philippe44]

Isn’t that code just equivalent to vec![data[0] as i16]?

I'm still a Rust noob and I had a hard time with the first PR that forced me to use Ref/RefCell/Arc/interior mutation :-(

[philippe44]

I kind of worry about passing the OGG data as i16s. For instance in this particular line, an extraneous byte gets inserted in the u8 -> i16 -> u8 conversion process.

I think it would be more appropriate to turn AudioPacket into a enum with a Vec entry for PCM samples and a Vec for OGG data.

I agree but this is always the last byte of a stream so it will be dropped. I did that b/c I did not want to have even more change to the rest of the code. I can study that option if you prefer

The pipe backend matches on the packet type and does the right thing. Other backends crash on OGG data.

[philippe44]

Looking at the pipe.rs code, it seems that it expects Vec

impl Sink for StdoutSink {
    fn start(&mut self) -> io::Result<()> {
        Ok(())
    }

    fn stop(&mut self) -> io::Result<()> {
        Ok(())
    }

    fn write(&mut self, data: &[i16]) -> io::Result<()> {
        let data: &[u8] = unsafe {
            slice::from_raw_parts(
                data.as_ptr() as *const u8,
                data.len() * mem::size_of::<i16>(),
            )
        };

        self.0.write_all(data)?;
        self.0.flush()?;

        Ok(())
    }
}

I understand I can modify that as well, but how much are the maintainers willing to let such modification creep too far in the code? I'm also having difficulties for now figuring out how AudioPacket can be an enum and not touch too much the rest of the code as well (like other sinks)

[sashahilton00]

I was just reading up on rust enums, they are pretty elegant. However, in the interests of expediency do we want to merge this and evaluate the audio packet changes in a separate PR - such a change looks like it will require some rewriting of the existing back ends.

[philippe44]

I think I have been able to use enum properly, well at least it works, produce the expected result and does not have too many ripple effects. The only one is the addition of a write_bytes() in the Sink trait that has a with implementation that does nothing, so no impact on other backends.

I've done per asked, a samples() function to access i16 and a bytes() function to access raw data. The passthrough is now properly writing Vec<u8> and not messing up with u16 and adding (in some case) an extra byte.

As said before, please excuse Rust rookie style mistakes. I also have an issue with the merger that I'm not sure I now how to fix. My cargo.lock should never have been pushed and the audio/Cargo.toml should just add Ogg, I don't know why it is in conflict now

[philippe44]

I don't think I can fix the --locked issue

[philippe44]

Any love/hate/opinion for that version?

[sashahilton00]

I'll try and fix the cargo.lock file tomorrow. There are a few PRs which are having problems due to older rust versions, to the point where it may make sense to just bump the MSRV to 1.42.0. With regards to the enums, @plietar is the enum implementation what you were expecting? Also, given the change to AudioPacket which is used in various backends, have they been tested to ensure that they are functioning as expected?

[plietar]

With regards to the enums, @plietar is the enum implementation what you were expecting?

Generally looks good. I'll have a proper look through the code this weekend.

[sashahilton00]

Are we good to merge this?

[plietar]

Could you add a high level description of how the passthru decoder works please.
In particular what is the purpose of the absgp stitching, and how does seeking work.

On every seek we re-send the same headers? Is that something decoders tolerate / require?

[philippe44]

Could you add a high level description of how the passthru decoder works please.
In particular what is the purpose of the absgp stitching, and how does seeking work.

On every seek we re-send the same headers? Is that something decoders tolerate / require?

I had to review things properly after that discussion. I was struggling too much with Rust, using Ogg backend at the same time. So there was a fair bit of remaining issues that are hopefully corrected now in addition of complying to most of your requirements.

A a general comment, this patch is to help the spotty project (https://github.com/michaelherger/spotty) where the main objective is to stream only one track per instance (with and optional starting offset) then launch another instance for the next track.

The passthru decoder nomally simply copies the entire stream to stdout, untouched. Now, there are a few problems because OggVorbis is not a fully streamable format as there are 3 headers (ident, comment and setup) per stream and they are all required especially the setup contains information to configure the vorbis decoder. Streams are divided into pages that have an own sequence number. There is no global header that tells the duration, instead the "granule" system tells the position of each page and seek has to deal with that (binary search I think). Players get the whole duration of a file by reading the granule of the last page of the file and look for serial stream number changes to detect number of streams.

1- When seeking within one track, headers must be added once at the beginning of the new position, so they must be gathered from the initial stream and then the seek must be done. Seek in librespot is done (AFAIK) by streaming the actual full data until the expected offset. I initially left some basic errors there as I misunderstood the way librespot chain tracks (see issue below) and there is no need to repeat the header when seeking, but the granule must always increment otherwise many players will terminate early. The remaining assumption is that all files have the same sample rate/granule unit

2- When chaining tracks (without user pressing next), I initially thought that librespot was continuing with the same decoder instance, but it's not the case. A new decoder is created for every track, even when they flow "naturally" but I had to create a fake serial stream number as Spotify always set it to 0 and players got confused at it would be the same stream then and granule would then have to be monotonic

3- Last, when user changes tracks, librespot also creates a new decoder instance but of course in that case there is no last page so the EndOfStream indicator is missing, which seems to be okay with all decoders I've tried, but still is a non-compliance to Ogg format. I don't think there is a clean workaround for that.

[sashahilton00]

There's quite a bit of stuff to be tweaked here before it's ready to merge just looking over the discussion. Can I propose we aim to get this in the 0.2.1 release? I'm conscious that the librespot-tremor issue for newer versions of rust is causing some problems for Spotifyd and would like to release a new version sooner rather than later.

[philippe44]

There's quite a bit of stuff to be tweaked here before it's ready to merge just looking over the discussion. Can I propose we aim to get this in the 0.2.1 release? I'm conscious that the librespot-tremor issue for newer versions of rust is causing some problems for Spotifyd and would like to release a new version sooner rather than later.

You are the maintainer, I'll do as you recommend. My only ask is to not lose "momentum" on the discussion

[philippe44]

I've addressed all the comments I could but I'm not sure why I have this conflict on player.rs. Let me know if you want to restart a fresh PR or squash things together

[Johannesd3]

There's quite a bit of stuff to be tweaked here before it's ready to merge just looking over the discussion. Can I propose we aim to get this in the 0.2.1 release? I'm conscious that the librespot-tremor issue for newer versions of rust is causing some problems for Spotifyd and would like to release a new version sooner rather than later.

@sashahilton00 Maybe apply/cherry-pick the tremor fix onto the current 0.1.3 and make a 0.1.4 of it?

[philippe44]

I've finally cleaned up (I hope for the best) the different commits and rebased to the latest dev to make differences easier to follow

[Johannesd3]

I have no objections anymore. While the static granule seems not ideal, I don't know a better solution either.

[philippe44]

I have no objections anymore. While the static granule seems not ideal, I don't know a better solution either.

Thank you for your comments - Rust is a steep learning curve :-)

[philippe44]

So I think I've made all the requested changes, including in all backends + error management. I hope this is a better "Rust-compliant".
I've also made modification in the granule management because I was wrong, the granule does not need to be monotonic across chained streams, it only needs to be within the same stream (when seeking). Still, it was not working before because I did not realize that spotify does not set the stream serial number and in that case, the Ogg stream seems to be a single one and there, a monotonic granule was required. I've just added a fake stream serial if it is set to 0

[Johannesd3]

It's great that the problem with the global granule is solved, thanks!

Maybe you could run cargo clippy and fix the warnings that are related to this PR?

[Johannesd3]

I added a few comments regarding error handling.

@sashahilton00 @ashthespy Do you think this would be an appropriate place to start using thiserror? It would spare us a lot of boilerplate here.

[philippe44]

Let me know if there is something else expected from me

[sashahilton00]

Just released 0.1.5 with the tremor fix, so there isn't pressure on this PR anymore. Will wait to hear from anyone that has any further thoughts/suggestions, otherwise this looks pretty much ready to go @philippe44

[sashahilton00]

@philippe44 would you mind squashing the commits? Also once this is merged you'll need to update the wiki to document the passthrough option. Additionally, if you could add a section explaining how the passthrough decoder works to the wiki, that would be great. The comment you left above should be good enough if you just copy/paste and tidy it up.

[Johannesd3]

May I point out that the commit has a not very meaningful name?

[sashahilton00]

If you could rename the commit to something more meaningful then that would be good. After that I'll merge it.

[Johannesd3]

@ashthespy (or @sashahilton00) After this is merged, could you try merging dev into tokio_migration, if you can spare the time? The more they diverge, the harder it will be to merge eventually.

[sashahilton00]

Merged. Please update the wiki accordingly as requested above.

[philippe44]

Merged. Please update the wiki accordingly as requested above.

Done - Thanks!