ParadoxSpiral / libmpv-rs

A libmpv abstraction written in rust that's easy to use and provides the ability to read next to all video and audio codecs.
GNU Lesser General Public License v2.1
100 stars 36 forks source link

Implement MpvNode (arrays and maps) #13

Closed yaymukund closed 4 years ago

yaymukund commented 4 years ago

I thought I'd take a stab at implementing MpvNode. It's very rough but it seems to return the expected keys.

Possible issues:

  1. ~I cast between i32 and usize~
  2. ~Should I be using MpvStr<'a> as the key to the HashMap~
  3. ~Is there a better way to construct the Vec/HashMap (for Array and Map respectively)?~
  4. ~Is try_into the correct approach here?~
yaymukund commented 4 years ago

Thanks for your comments. I'll update this soon ^^

yaymukund commented 4 years ago

I think this is ready for another review. I ended up changing things quite a bit to avoid allocations, but we now return an MpvNode which owns the data and exposes the data via fn value(&self) -> MpvNodeValue<'_>. I think this is zero allocations?

I was discussing this in the Rust community discord with moxian who helped design this (+ others! :), and also suggested possibly doing:

impl MpvNode {
    fn ref_value(&self)->MpvNodeValueRef<'_> { ... }
    fn value(&self)->MpvNodeValueOwned { ... }
}

I'm not entirely sure what that buys us, but thought I'd flag it up :)

yaymukund commented 4 years ago

Thanks for your review. After reading some other discussions about how to do this [1], I feel good about the approach we've taken in this PR.

[1] Related:

https://users.rust-lang.org/t/how-do-i-get-rid-of-this-pattern-of-extracting-a-value-from-an-enum/12555 https://stackoverflow.com/questions/34953711/unwrap-inner-type-when-enum-variant-is-known

ParadoxSpiral commented 4 years ago

Thanks for your work!