Allow access to mutable writer in PacketWriter

[philippe44]

Hi - I have a management struct who receives (and seeks) an Ogg source using a PacketReader and rewrite it using a PacketWriter, but it's not exactly like repack. In that case, the result of packet writer must in be some internal memory or an object that must be read by another part of the code who really takes care of moving these packets to destination.

So I created a PacketWriter with a Vec (buffer) for its 'wtr' and that's fine, it provides the traits for being writable. Unfortunately, this PacketWriter borrows permantely the 'wtr' and I cannot read it anymore. Otherwise, I'd drain() it and shrink_to_fit() nut here data just accumulate.

If I call into_inner, I move out the wtr which is good to let me access the Vec but not desired as I can't put it back later in the PacketWriter, with the changes I've made. My only option (with my limited Rust understanding) is to borrow 'wtr', drain and empty it and release it after, hence this PR.

This is single thead adn the flow is in a loop

• Read Packet from an external io::Reader
• Write new Packet using Vec
• Send data from Vec somewhere else
• loop

I'm a Rust Noob but I've tried...

[est31]

I'm uneasy with exposing the internal writer, as it may expose the internal behaviour of different PacketWriter functions. So I'm not sure if this fits the separation of concerns well. I've checked your PR https://github.com/librespot-org/librespot/pull/560 . Not sure that what you want to do can't be done through other means as well. Furthermore I'm not sure your approach in that PR will work, as it assumes it can just take what the ogg crate returns. Those data first still have to be read by the vorbis crate though before they can be turned into audio. Have you tested that PR?

[philippe44]

Yes I have tested it fine. The Ogg crate returns a Vec because the writer has been created with that. So I know I can do drain() and a few simple things on that Vec and that's all I need. I've played various spotify files and compared them bit exact, from stdout.

But I don't want to turn them into audio, I don't need the Vorbis crate, I just need the Ogg crate to extract the Ogg and play with headers. I don't touch the Vorbis encoded audio, just the Ogg packetization (it's a passthrough)

In fact, I'm not doing much more than what your ogg "repack.rs" does, except that I don't have access to the final io object for writing, I can only return a Vec

[est31]

@philippe44 but why do you do this:

            // we need an even number of bytes to map to i16
            let data = self.wtr.inner_mut();
            let len = data.len() / 2;

            if len > 0 {
                //eprintln!("O-Len: {:?}", &len);
                let data16 = unsafe { slice::from_raw_parts(data.as_ptr() as *const i16, len) };
                let result = AudioPacket(Vec::<i16>::from(data16));

                data.drain(0..2 * len);
                data.shrink_to_fit();

                return Ok(Some(result));
            }

Why do you cast into a i16 slice???

[philippe44]

Sadly, librespot expects 2-bytes per item as it normally handles i16 for device drivers or stdout. I can't send u8 which is the base of work for the packet reader/writer. So I need to make that ugly cooking to get my i16 from my u8. I'm not sure about alignment in a sense that it might be a risk but in many month of playback on difference machines, it never happened,.

Now, if there it a better way, I'm happy to change it. As said, I'm just starting with Rust and got all my bad habbits from 30 years of C (and C++, but less) and Perl

[est31]

@philippe44 Note it's fine to do such a choice. I was just a bit confused as i16 seems like it's decoded samples, which it's not :).

[philippe44]

Thanks!

[philippe44]

One question if you don't mind: it's true that the LICENSE.BSD file might be un-necessary as the amount of code from writer.rs is very small (see https://github.com/librespot-org/librespot/pull/560/files#diff-4f83a6c6e171360de4308e08ff22d6bace86301ed8dd8499bcaa5cea634bb7f6). That would allow me to have one less file in the commit. Now, if you don't want to, I'll leave it there

[est31]

@philippe44 can you squash your PR and remove the file? Then I can take a look whether it's fine.

[philippe44]

Done - I had a bit of a disaster moment with squash...

[philippe44]

Folks @librespot were asking me to have my patch using your modification to access the ref internal object. DO you plan to push a release soon so that we can refer by a version's tag?