Fix segfault reading input packet stream (compiler type layout change?)

[projectgus]

• Fixes #176 (see linked issue for a description of what I think the problem there was.)
• Adds an explicit copy_for_wrap() function to avoid the possibility that transmute_copy() has input or return types other than Context.

I'm not that comfortable with unsafe Rust or libavcodec, but hopefully this works as expected. :)

[tilpner]

Adds an explicit copy_for_wrap() function to avoid the possibility that transmute_copy() has input or return types other than Context.

Instead of wrapping the transmute(s) in a function, to constrain the type parameters to those of your wrapper function, you can also specify the types explicitly:

diff --git a/src/format/context/input.rs b/src/format/context/input.rs
index 8d423bb..c2dbdf5 100644
--- a/src/format/context/input.rs
+++ b/src/format/context/input.rs
@@ -181,10 +181,10 @@ impl<'a> Iterator for PacketIter<'a> {
                        Err(Error::Eof) => None,

                        Ok(..) => unsafe {
-                               Some(Ok((
-                                       Stream::wrap(mem::transmute_copy(&self.context), packet.stream()),
-                                       packet,
-                               )))
+                               let context = mem::transmute::<&Context, &Context>(self.context);
+                               let stream = Stream::wrap(context, packet.stream());
+
+                               Some(Ok((stream, packet)))
                        },

                        Err(err) => Some(Err(err)),

Having a copy_for_wrap function publicly available is a bit awkward (even though it's unsafe). It doesn't actually even copy the Context, it just extends the lifetime of a reference to a Context.

I think I'd prefer the explicit transmutes, like in f41149fd951e01d5114b7bffdeeb9feb6e767942, because they don't introduce any new public API, and it makes explicit at each callsite that lifetime extension is happening (whereas it would be slightly obscured with copy_for_wrap).

I am wondering, given a Context is a libavcodec pointer and a reference counted destructor, if it would be better to implement std::clone and store clones of Context in Streams instead of storing an unsafe reference.

I can only speculate, but because a separate Stream is passed for every input packet, cloning the context would cause a new Rc for every input packet, which might have felt unnecessarily expensive during the initial implementation.

[projectgus]

@tilpner Thanks for the detailed response, I appreciate it! The commit you linked looks good, and I can see the argument for keeping the unsafe operation explicit rather than hiding it.

I can only speculate, but because a separate Stream is passed for every input packet, cloning the context would cause a new Rc for every input packet, which might have felt unnecessarily expensive during the initial implementation.

Fair enough, makes sense! If I'm feeling ambitious I'll try to implement this and benchmark it out of interest, but I think you're probably right and it's implemented like this for a reason. :grin:

[tilpner]

Thank you for the contribution! :)