search

lisamelton / video_transcoding

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

Ensure that data provided by mp4track --list is valid UTF-8 #152

Closed DavidNielsen closed 7 years ago

DavidNielsen commented 7 years ago

If MP4track —list returns anything containing a ReadAtom warning in addition to track listing, such as:

ReadAtom: "testfile.mp4ا?" 3976103409 vs 3958522092s outside parent atom - skipping to end of "" " track[0] id=1 type = video enabled = true inMovie = false inPreview = false layer = 0 alternateGroup = 0 volume = 0.0000 width = 1280.00000000 height = 720.00000000 language = English handlerName = Mainconcept Video Media Handler userDataName = track[1] id=2 type = audio enabled = true inMovie = false inPreview = false layer = 0 alternateGroup = 0 volume = 1.0000 width = 0.00000000 height = 0.00000000 language = English handlerName = Mainconcept MP4 Sound Media Handler userDataName =

Then invalid input is provided to transcode-video which then fails with:

transcode-video -v --handbrake-option encoder=x265 --target small testfile.mp4 transcode-video 0.17.3 Copyright (c) 2013-2017 Don Melton HandBrake 1.0.7 found... Processing: redacted.mp4... Scanning media title 1 with HandBrakeCLI... Scanning with mp4track... mp4track - MP4v2 2.0.0 found... /usr/local/bin/transcode-video: invalid byte sequence in UTF-8

This prevents this unfortunate misery.

lisamelton commented 7 years ago

@DavidNielsen Thanks for the patch! Sounds like a really annoying bug, too. I'm traveling today so I probably won't get a chance to review it in depth for several more hours.

A few questions now: on which OS did this happen? And can you upload the input file somewhere (if it's small enough) so I can examine it?

Thanks!

DavidNielsen commented 7 years ago

This happens on macOS 10.12 (and earlier versions).

I sadly can't upload any of my test files for reasons of size and copyright, which I hope is understandable. I'll see what I can do about producing a suitably small one which can be shared.

As for a review, I have near zero competency with regards to programming so feel free to focus on the traveling. I merely added the same adjustment to mp4track as was present already for handbrake, reasoning that it would apply equally. Since the problem went away after utterly minimal testing, it is officially pull request time.

lisamelton commented 7 years ago

@DavidNielsen Cool and I completely understand why you can't upload that.

I thought that code snippet looked familiar. :) Bonus points for reusing it!

lisamelton commented 7 years ago

@DavidNielsen OK, this should work just fine and fix the weird bug you found.

But please delete the blank line between io.each do |line| and line.encode! since that's not necessary. Once you make that change I will gladly merge it.

lisamelton commented 7 years ago

@DavidNielsen Of course, if you want, I can merge your patch as is and then just make the edit myself. Let me know.

DavidNielsen commented 7 years ago

I won't be near my Mac till tomorrow to do the edit. Probably quicker to merge now.

lisamelton commented 7 years ago

@DavidNielsen Okey dokey. I'll probably do that later tonight then. No worries and thanks!

lisamelton commented 7 years ago

@DavidNielsen I just noticed a bug in your patch after I merged it. You're sending the same debug output to the console twice. So I'm fixing that now.

lisamelton commented 7 years ago

@DavidNielsen OK, the fix is also checked in. It was simple — just deleting the last Console.debug output line. No worries. I should have caught it in the code review. That's what I get for drinking my lunch. :)

lisamelton commented 7 years ago

@DavidNielsen BTW, I probably won't release this change until late next week. I may have a few other things on the plate I might decide to include with it.