search

lisamelton / video_transcoding

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

buffer multibyte characters #264

Closed joshstaiger closed 5 years ago

joshstaiger commented 5 years ago

Buffer multibyte UTF-8 characters to prevent crash detailed in

https://github.com/donmelton/video_transcoding/pull/263

lisamelton commented 5 years ago

@joshstaiger Thanks for the the quick patch! That's not the way I would format the code but I can't argue with the speed. :)

Does that work for you, @samhutchins? And should we replicate something like this elsewhere?

BTW, I can't test any of this until I get back to my development machine this evening.

lisamelton commented 5 years ago

@joshstaiger And a quick update for the end-of-stream issue! You're on a roll, sir. :)

joshstaiger commented 5 years ago

A little too quick ;)

Anyway, I'll let you guys take over. I've tested on a couple different encodings that I've had trouble with in the past and it seems to work okay.

Feel free to reformat the code however you'd like.

samhutchins commented 5 years ago

Buffering seems to work based on my quick tests, I don't have any sources that expose the bug handy though

No console spew with this PR, which is nice :-)

lisamelton commented 5 years ago

@joshstaiger @samhutchins It looks like I won't be able to test this until late tomorrow afternoon or evening due to family commitments. My apologies.

But if this fixes both problems and doesn't cause side effects with progress feedback or log file consistency then I want to take the change.

lisamelton commented 5 years ago

@joshstaiger @samhutchins I managed to squeeze in a quick test in the midst of family commitments and... it seemed to work fine! No side effects.

But I won't be able to test it thoroughly until much later today. And if it passes those tests I'll merge it, reformat it and then look at applying the same change to other places in the code.

Thanks!

lisamelton commented 5 years ago

@joshstaiger I squashed and merged your patch but for some strange reason your revision to flush the buffer wasn't included. Go figure. However, I restored it when I revised your nomenclature and formatting. Please review those changes to make sure it's all there.

Also, please review the changes to see how I reformatted your code. Why? Because I want you to understand the code style because I hope to get more excellent patches like this from you in the future, sir! And I'm a style nazi. Talk to @samhutchins, he has lots of experience with my quirks. :)

Thanks, again! This will get released sometime next week.

lisamelton commented 5 years ago

@joshstaiger OK, I'm an idiot. :) I made a cut and paste error when revising your code. Your revision to flush the buffer was there. And everything should be fixed now. Sorry for the confusion.

joshstaiger commented 5 years ago

No worries. I've done worse :)

After the fix, the logic looks correct and the formatting looks entirely reasonable.

Will plan to retest after release.

lisamelton commented 5 years ago

@joshstaiger OK, this has now been released in version 0.25.0 of the Gem. Thanks again for all your help!

klogg416 commented 5 years ago

@joshstaiger @donmelton Did a quick test run:

 + audio tracks:
    + 1, English (FLAC) (2.0 ch) (iso639-2: eng)
    + 2, 日本語 (FLAC) (2.0 ch) (iso639-2: jpn)
  + subtitle tracks:
    + 1, English [PGS]
    + 2, English [PGS]
    + 3, English [PGS]
    + 4, English [PGS]
    + 5, 日本語 [PGS]

Look at that glory! Thank you @joshstaiger for this fix and making WSL less predictably unpredictable.

lisamelton commented 5 years ago

@klogg416 And thanks for testing, sir!