search

lisamelton / video_transcoding

Tools to transcode, inspect and convert videos.
MIT License
2.39k stars 160 forks source link

Proposal to require HandBrakeCLI version 1.1.0 for access to new JSON API #234

Open lisamelton opened 5 years ago

lisamelton commented 5 years ago

Proposal to require HandBrakeCLI version 1.1.0 for access to new --json API

The transcode-video, convert-video and detect-crop tools share code to analyze media inputs. This is necessary to understand how that media is configured. The implementation of that code parses the unstructured text output from HandBrakeCLI via its --scan option.

And that implementation is extremely fragile because the format of the unstructured text from HandBrakeCLI, and indirectly libAV, keeps changing.

In fact, since version 0.18.0 of the video_transcoding Gem, language detection for subtitles in disc image directory input and individual closed caption tracks may still be wrong due to some of those changes. And that's only one example of the breakage.

I've contacted the HandBrake development team about this problem and it's their position that they will never address the fragility of their unstructured text output. Instead, they're encouraging developers to parse the structured text output available in a JSON-like format via the --json option in HandBrakeCLI version 1.1.0 or later.

So, I propose that we make that version of HandBrakeCLI a minimum requirement and completely rewrite our media analysis code to parse the JSON output.

Why not keep the fragile implementation and keep supporting older versions of HandBrakeCLI?

Because maintenance and testing of both code paths would be onerous and risky. And I'm not willing to sign up for that work or headache.

There are other benefits to only supporting the JSON path. First, based on some of my experiments, the media analysis implementation will be significantly simpler and smaller. Also, it's possible that we can get rid of the dependency on mp4track and the entire mp4v2 package.

Version 1.1.0 of HandBrakeCLI was released on April 8, 2018, so it's been available for about nine months. Most macOS and Windows users are already on version 1.1.2, two revisions newer. My only concern about making this change is for Linux users. Because some Linux distributions (I'm looking at you Debian) take years to include newer versions in their packages.

Let me know what you think. I won't be doing this right away, but I figured we should start discussing it now.

Thanks.

khaosx commented 5 years ago

I don't have a problem with it. I do hate the notion of potentially dropping support for some folks, but on the other hand, this is a one-man show where we all benefit from your labor. I can't see many folks here arguing that you should have to do more workarounds to compensate for a shortcoming in HandBrake when there is a potentially better/simpler method.

lisamelton commented 5 years ago

@khaosx Thanks! Yeah, the duct tape and bandaids in the current code are already making it difficult to maintain.

klogg416 commented 5 years ago

I am all in. Based on absolutely no research, I suspect most Linux users (self include!) who are interested enough in transcoding to have found your scripts, have long ago manually added the direct handbrake repository to their package sources to get the benefits of new releases.

On Nov 30, 2018, at 9:43 PM, Don Melton notifications@github.com wrote:

@khaosx Thanks! Yeah, the duct tape and bandaids in the current code are already making it difficult to maintain.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

lisamelton commented 5 years ago

@klogg416 That's what I was hoping Linux users were doing! Thanks for the feedback.

samhutchins commented 5 years ago

On the Linux front, the handbrake team pretty strongly recommend using the builds they provide and not the ones that get packaged for distro repositories.

I’m up for the change, and dropping the dependency on mp4v2 would be a win for Windows users, as there are no official Windows builds for that

lisamelton commented 5 years ago

@samhutchins I didn't realize the HandBrake team did that. God bless them! :) And thanks for the feedback! I suspected you would be happy to get rid of that dependency.

RodBrown1988 commented 5 years ago

Definitely a +1 from my end. Fragile solutions are just a pain in the rear end, and I think most of us enthusiasts are incessantly upgrading just for fun and testing anyway, so makes sense.

lisamelton commented 5 years ago

@RodBrown1988 Thanks! Yeah, most of us nerds are living on the bleeding edge. :)

vr8hub commented 5 years ago

I am not on Linux so wouldn't be negatively impacted, but I am all for anything that makes things simpler for you and better (less fragile) for the code.

lisamelton commented 5 years ago

@vr8hub Thanks!

BTW, this is taking longer to complete because it's actually harder to simplify than I thought. And that irony isn't lost on me. :)

genimac commented 5 years ago

After Debian Buster release the version on the repositories is 1.2.2+ds1-1 for stable, testing and sid. The current one in deb-multimedia.org is 1:1.2.2-dmo2. The PPA from Hanbrake page give 1:1.2.1-zhb-1ppa1~cosmic1 So no problem which repository you use, you end at 1.2.2 (Hanbrake PPA nomenclature differs)

lisamelton commented 5 years ago

@genimac Thanks for the info!

bloertscher commented 3 years ago

I have made an initial attempt at implementing this if you want to take a look #333

lisamelton commented 3 years ago

@bloertscher Thanks. I'll take a close look in the next few days.