search

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

Potential merge with Cobrand/mpv-rs ? #1

Closed Cobrand closed 4 years ago

Cobrand commented 7 years ago

Hey, I've stumbled on that repository by pure luck, I didn't know someone else was doing an mpv binding library beside me.

I don't know if this repository is still in active development but ... if it is I think developing the same thing twice but in different repositories is not a great idea, how about we discuss things a bit about how we could merge/override in one way or another the repositories together so we don't end up developing the same thing twice in different ways ?

Cheers

ParadoxSpiral commented 7 years ago

Hi. Sorry for the late reply, I was on vacation.

I saw your crate only very recently as well via github, and my sdl2 example is basically derived from yours^^.

This is still in developement, but I'm quite happy with the current state. The only major thing unfinished is fixing the broken build script, which I have been trying for some time now, but can't figure out, and a weird spurious bug that I just discovered :(.

I skimmed your code, and it seems less abstract. I focused on converting libmpv constructs into more idiomatic rust versions, e.g. EventIter in events.rs, and protocol.rs and its example.

I'm not sure if one approach is better that the other, but it seems to me that my version is more mature. Basically all low level functionality is available by using the mpv handle and the $crate::raw module.

If you want to discuss this in a more instant kind of way, would a temp channel on Mozilla's irc be okay?

Cheers

Cobrand commented 7 years ago

Sorry for the delay, I've had my share of things to manage lately. I'd love to discuss things with you ! ... but it looks like you live in Germany, and I currently live in the west side of the US, so it effectively is a 9 hour time difference. I don't mind the IRC, but I'd prefer if we could have a way to send asynchronous messages and having a way to see days-old or months-old messages as well. If you have an idea, you can email me a solution (you will find my email address in any of my commits).

Just a few things that I've noted in case we are in for a merge:

Overall I do prefer your version, but I feel like there are little details and misc things that we have to set up in order to make a successful merge.

On a side note, our licenses are different as well, and I do prefer Apache-2.0/MIT since it's more permissive and aligned with almost every other crate's license (as well as Rust's license), but this is arguable and only a minor matter.

ParadoxSpiral commented 7 years ago

Thanks for the feedback!

If an overhaul is needed, the ideal solution is that we separate mpv-rs which would be a wrapper, and mpv-sys which would be the equivalent of your "raw" module, but automatically generated with rust-bindgen with a build.rs file.

I like the idea of the seperate crate, but I don't really like bindgen. For one I don't like how it formats the types, but more importantly I don't want all the deprecated enum values and functions to be included.

I didn't quite get how to use your events, but at least you have a way to use them, where I don't.

I do have to write better documentation. Anyways, first you observe events with [Parent|Client].observe_events(&[Event]), this returns an EventIterator, which implements Iterator. Different iters can observe different sets of Events at the same time. The next method (from the iter trait) blocks the caller until events are available, and then returns them as a Vec<Event>. I hope this makes sense :)

When I created the bindings, there wasn't any "stream_cb.h" file, hence that's why I don't have your version of "Protocol", and I don't even a clue of what does it do.

Basically, you can register a custom callback based protocol. Protocol here refers to the prefix, e.g. filereader:///path/to/file. Each custom protocol must provide 3, and 3 optional callback function: One that opens the stream and is called once when the resource is loaded, one that closes the stream and is called once the resource is closed, one that is called when libmpv needs to read data from the resource, an optional one that is called when a seek is requested (resource may not support seeking), and one optional that returns the size of the resource (resource may not have a known size).

My implementation takes care of the danger of panicking when called from C code, by providing wrapping functions. This also makes the parameters etc. more idiomatic.

While our ways of setting properties (and options) are alike, I[...]

Working on this in a branch.

I like the fact that you included a build.rs which builds libmpv, and that ts is only enabled if the specific feature is enabled as well. However I didn't test it but it looks like it's trying to build (ie download, build, link, ...) everytime you try to build this crate, is that right ? I think it would be nice to add a feature that only builds libmpv if it isn't already on the system; if it's already there, the build is ignored.

It only downloads if there isn't a downloaded version already, but it always rebuilds (on unix). I can fix this though. I'm not really sure if this is worth the maintenance effort though, because this is really the job of the packet manager. Then again it's nice for cross compiling..

On a side note, our licenses are different as well, and I do prefer Apache-2.0/MIT since it's more permissive and aligned with almost every other crate's license (as well as Rust's license), but this is arguable and only a minor matter.

I very much prefer GPL because of its requirement to publish patches etc. In a sense, non libre licenses are more restrictive because they don't guarantee the four freedoms.

EDIT: Also a drawback that needs to be fixed, is that the crate requires nightly features.

Cobrand commented 7 years ago

If an overhaul is needed, the ideal solution is that we separate mpv-rs which would be a wrapper, and mpv-sys which would be the equivalent of your "raw" module, but automatically generated with rust-bindgen with a build.rs file.

I like the idea of the seperate crate, but I don't really like bindgen. For one I don't like how it formats the types, but more importantly I don't want all the deprecated enum values and functions to be included.

Most of the crates being *-sys are like that, but I can understand where you're coming from with the deprecated functions and the formatting. However, bindgen does one thing very well compared to hand-crafted bindings: it's never wrong (unless bugs of course but I've yet to experience one). So what I propose is to generate the mpv bindings with bindgen once by command line, format/rename the types however you like, and then list the exact command line + all of these format/comments/removals to have a way to generate the mpv-sys crate again. mpv is changing quite fast, I'm sure that in a few months or years other functions will be deprecated and new functions will appear as well; we should have a way to re-generate the binding whenever we want without breaking crates that use this module by renaming functions/structs in another way once again.

On a side note, our licenses are different as well, and I do prefer Apache-2.0/MIT since it's more permissive and aligned with almost every other crate's license (as well as Rust's license), but this is arguable and only a minor matter.

I very much prefer GPL because of its requirement to publish patches etc. In a sense, non libre licenses are more restrictive because they don't guarantee the four freedoms.

I don't get your point about non-libre licenses, but I'm fine with LGPL-2.1 as long as we don't go the GPL way or the LPGL-3 way. As long it's usable by all free or non-free software, I don't really care about the license underneath.

ParadoxSpiral commented 7 years ago

Most of the crates being *-sys are like that, but I can understand where you're coming from with the deprecated functions and the formatting. However, bindgen does one thing very well compared to hand-crafted bindings: it's never wrong (unless bugs of course but I've yet to experience one). So what I propose is to generate the mpv bindings with bindgen once by command line, format/rename the types however you like, and then list the exact command line + all of these format/comments/removals to have a way to generate the mpv-sys crate again. mpv is changing quite fast, I'm sure that in a few months or years other functions will be deprecated and new functions will appear as well; we should have a way to re-generate the binding whenever we want without breaking crates that use this module by renaming functions/structs in another way once again.

Ok, seems like a good way to do it.

I don't get your point about non-libre licenses, but I'm fine with LGPL-2.1 as long as we don't go the GPL way or the LPGL-3 way. As long it's usable by all free or non-free software, I don't really care about the license underneath.

If there are any problems in the future I'd be willing to relicense then.

Cobrand commented 7 years ago

Looks like it's settled then. We should definitely keep your version, modify it slightly so that it reflects what we've talked about here. Also, it needs more examples and documentation, but I feel like this will be the very last thing to do before publishing on crates.io

Just one last thing, you mentioned using nightly features in your edit; while I see why you used untagged_unions (I didn't search thoroughly but the C API uses one right ?), I don't get why you need alloc, core_instrisics and heap_api, care to explain ?

ParadoxSpiral commented 7 years ago

Just one last thing, you mentioned using nightly features in your edit; while I see why you used untagged_unions (I didn't search thoroughly but the C API uses one right ?), I don't get why you need alloc, core_instrisics and heap_api, care to explain ?

I just removed untagged_unions, because it was used for MpvNode which was not really supported and will be moved to the mpv-sys crate anyways. core_intrinsics I use for a branch prediction hint, but premature optimization is evil and all.. it can probably removed. alloc and heap_alloc is used in protocol.rs to allocate an uninitialized value that is later set to the return value of the StreamOpen callback. This can probably be made to work with the unsafe mem functions.

Cobrand commented 7 years ago

When you think you are done with your changes and fixes, add me as a collaborator so I can begin to change what we've discussed.

Also, this repo needs a travis/appveyor to make sure tests pass everytime. I think I'm average with travis, but for some reason I've already tried to install travis for mpv-rs on my end and it has been a pain every time (mpv as so much dependencies it's crazy).

ParadoxSpiral commented 7 years ago

I haven't figured out #2 yet, but I added you so that you can start contributing whenever you want.

Never used travis before, but I'll give it a shot.

Cobrand commented 7 years ago

I will be on rust's IRC from 6pm UTC to 11pm UTC, feel free to ping me so we can talk about stuff more easily.

marioortizmanero commented 4 years ago

I asked @Cobrand about the situation today at https://github.com/Cobrand/mpv-rs/pull/5#issuecomment-644721486. He won't be working anymore on his own version, so @ParadoxSpiral, you may upload it to crates.io if you're interested on working on your fork in the future.

I'm not sure how many changes you've made to your fork already, but it'd be a good idea to move more documentation to this repo, possibly from the README.md. Also, the original version should point to this one and perhaps be archived, so that future users can find this fork easily.

Cobrand commented 4 years ago

@marioortizmanero just so you know, this is not a fork: the author made a new crate from scratch with a wildly different architecture and different features included. Aside from the fact they are both for mpv they are wildly different.

I agree with everything else you've said though, and as mentioned in the other issue I am willing to give the crates.io/mpv rights to anyone who asks directly.

ParadoxSpiral commented 4 years ago

I was considering renaming this crate to libmpv-rs & libmpv-sys, I am not sure if it is good form to just upload a different crate under the same name to crates.io as @Cobrand also noted here https://github.com/ParadoxSpiral/mpv-rs/issues/7#issuecomment-547959316.

I haven't had much/any time to work on this crate, but I do have some breaking changes coming that remove the overengineered events_sync and simplify events_simple so it would be a good time for the rename and breaking changes to happen at the same time.

ParadoxSpiral commented 4 years ago

This crate has since been published to crates.io under the name libmpv and libmpv-sys with a few architectural changes. As said I don't think this crate can be merged with the other at this point anymore, so I am closing the issue. Though maybe I'll snatch some documentation from you ;)

marioortizmanero commented 4 years ago

What about changing the name of this repo as well to fit what's on crates.io?

Also, the packages on crate.io don't seem to have a repository attached (they're usually at the top, next to "Documentation"), can you add this repo?