This PR seems to include some files from my BSD licensed crate. It doesn't seem to comply with the BSD license.

This PR seems to include some files from my BSD licensed crate. It doesn't seem to comply with the BSD license.

I think I just copied the writer.rs, renamed it ogg_writer.rs, added the inner_mut fn and that's all. Happy to change anything else that I might have forgotten

@philippe44 the issue is that the LICENSE file referenced at the head of the file is different. You should create a LICENSE-BSD with this as the content and then change it to LICENSE-BSD, at least for that file you copy.

What I still don't get: why not

1. move out the PacketWriter by doing mem::swap with a dummy one
2. call into_inner on the moved out PacketWriter
3. manipulate the internal Vec to your choosing
4. create a new PacketWriter from that Vec and mem::swap it into place

@philippe44 the issue is that the LICENSE file referenced at the head of the file is different. You should create a LICENSE-BSD with this as the content and then change it to LICENSE-BSD, at least for that file you copy.

That part is done

What I still don't get: why not

1. move out the PacketWriter by doing mem::swap with a dummy one
2. call into_inner on the moved out PacketWriter
3. manipulate the internal Vec to your choosing
4. create a new PacketWriter from that Vec and mem::swap it into place

That what I did at the beginning, but I was losing the rest of the context of the PacketWriter (the hasmap) when I was pushing it back and so the file was incorrectly coded.

Don't get me wrong, I'm a Rust rookie so I've probably made some non-elegant choices, but I tried that to start with. Maybe not correctly.

That what I did at the beginning, but I was losing the rest of the context of the PacketWriter (the hasmap) when I was pushing it back and so the file was incorrectly coded.

Fair point!

Don't get me wrong, I'm a Rust rookie so I've probably made some non-elegant choices, but I tried that to start with. Maybe not correctly.

It's ok. I hope I'm not annoying you :).

As we've established that re-building a PacketWriter loses context, what about this different idea: usage of a wrapper struct like WrappedWriter(Rc<RefCell<Vec<u8>>) that you impl the Write trait on. Then you can keep the Rc around and manipulate it freely.

It's ok. I hope I'm not annoying you :).

Not at all, that's a learning moment for me!

As we've established that re-building a PacketWriter loses context, what about this different idea: usage of a wrapper struct like WrappedWriter(Rc<RefCell<Vec<u8>>) that you impl the Write trait on. Then you can keep the Rc around and manipulate it freely.

So, I'll have to dig into Rc & RefCell which I have not yet - I just started Rust 3 days ago :-). I looked at these and I was already struggling with the borrow-checker and lifetimes and concepts/syntax in general, so I left that part for later. I will look at it tomorrow (it late for me now)

Thanks!

Well ... there is always something else. I've managed to use Rc & RefCells with a wrapper, per @est31's suggestion like that

pub struct WrappedWriter {
    data :Rc<RefCell<Vec<u8>>>,
}

impl WrappedWriter {
    pub fn new(data : Rc<RefCell<Vec<u8>>>) -> Self {
        return WrappedWriter {
            data,
        };
    }
}    

impl Write for WrappedWriter {
    fn write(&mut self, buf: &[u8]) -> Result<usize, Error> {
        let mut data = self.data.borrow_mut();
        data.write(buf)
    }
    fn flush(&mut self) -> Result<(), Error> {
        Ok(())
    }
}

Just to be hit by the next problem with is that player.rs spawns (and move) the player's context so although everything actually moves and it's thread-safe, Rust compiler refuses to compile because of use of Rc. So I don't have any other solution for now

(sigh)

@philippe44 you can try using an Arc instead of an Rc. That should be enough.

@philippe44 you can try using an Arc instead of an Rc. That should be enough.

Unfortunately the same error happens with RefCell

Hmm then maybe a Mutex is needed instead of the RefCell.... sad that it can't be all moved over in one go. I think this is some limitation of the Rust type system...

... well using Arc<Mutex<Vec>> worked ... this is a real pain and probably a lot of un-needed consumption of mutexes & al. At least, I had a crash course on Rc, RefCell, Arc, RwLock and Mutex in Rust

Yeah it's not an optimal solution.... I think I'll just merge your PR. This solution should be good in the meantime though.

Also it might be a good idea to squash the PR as it reduces its noise. And remove the LICENSE-BSD file :).

Yeah it's not an optimal solution.... I think I'll just merge your PR. This solution should be good in the meantime though.
Also it might be a good idea to squash the PR as it reduces its noise. And remove the LICENSE-BSD file :).

Re LICENSE-BSD, some of the passthrough-decoder comes a bit from the "writer.rs", so I can do as you prefer. About the squash, some maintainers don't like that, I'll wait for a response I guess

Thanks! Consider my case with the LICENSE-BSD file rested.

Thanks!

@sashahilton00 @plietar - this patch is now ready for consideration. I can update it if you want to integrate the latest version of @est31 https://github.com/RustAudio/ogg who kindly incorporated my patch, but it's not mandatory, I've found a workaround using Rc/Arc+RefCell/Mutex/RwLock.

That would be really great to have that option native to help the Spotty project!

Thanks

@sashahilton00 - any opinion on this new "Ogg Direct" implementation? Any chance to get it merged?

The tests might need some tweaking.

I'm happy to merge this, but someone needs to test it to make sure it's all ok, since I have nothing that uses pass through. Additionally, now that the ogg patch has been merged upstream, it would be good if that were used instead, since we're trying to minimise the amount of audio related logic that happens within librespot itself.

I've done tests on my side prior submitting PR. I'm happy to use the updated Ogg version, as long as it is synchronized with librespot (pointing to that version). I'll also commit to maintain & support these changes

Sounds good, feel free to make the necessary changes and update the PR. It looks like you might need to rebase or otherwise update the Cargo.lock file, since it's failing CI tests currently

Ok, will do. I've contacted @est31 to see if he agrees to tag a new release so that we can point "officially" at it

@philippe44 new release is out!

Closed for the benefit of #569

@philippe44 new release is out!

Thx!