Add --reverse-double-order to transcode-video

[samhutchins]

I currently use --mp4 with transcode-video, in part because it's what I'm familiar with, but also because it puts the aac stereo track first. I want the aac track first so that Plex uses it by default, which means it doesn't have to transcode the audio. With this --reverse-double-order feature I'd be able to start transcoding into the mkv container, which I'd like to do so I can more easily manipulate the metadata after the fact.

[samhutchins]

Eh, ignore this, I didn't consider adding multiple audio tracks like commentaries.

I feel a fool

[lisamelton]

@samhutchins Actually, I don't think multiple audio tracks would be a problem because, as I envisioned the feature, it would only reverse the order of double-width output track pairs. Plus, if no other output tracks were double width except main, then this should work for you.

Are you sure you want to close this?

[samhutchins]

I might re-open with a different patch, but I think the current patch will put commentary tracks before the main audio if you include --reverse-double-order. I'mma take some time to do proper testing, I rushed this a little in my excitement.

[lisamelton]

@samhutchins Yeah, it would. And I couldn't take the patch then. It's early here and I didn't realize this was a pull request and not just a feature request. :)

[lisamelton]

@samhutchins BTW, I know exactly where in the code to make the modification to implement this feature. Search for when :double in the transcode-video source file and you'll see it too. :)

[samhutchins]

Something like this? (ignore the double '' I put in the help text, I did it to fix syntax highlighting in VS code)

diff --git a/bin/transcode-video b/bin/transcode-video
index d6e5e58..b659f5e 100755
--- a/bin/transcode-video
+++ b/bin/transcode-video
@@ -147,6 +147,11 @@ Audio options:
                       (can be used multiple times)
     --aac-encoder NAME
                     use named AAC audio encoder (default: platform dependent)
+    --reverse-double-order
+                    reverses the order of `double` width audio tracks, placing
+                      the AAC stereo track before the AC-3 surround track in 
+                      mkv output, and the AC-3 surround track before the AAC
+                      stereo track in mp4 output
     --no-audio      disable all audio output

 Subtitle options:
@@ -163,7 +168,7 @@ Subtitle options:
                       (language code must be ISO 639-2 format, e.g.: `eng`)
                       (multiple languages are separated by commas)
                       (can be used multiple times)
-    --no-auto-burn  don't automatically burn first forced subtitle
+    --no-auto-burn  don''t automatically burn first forced subtitle

 External subtitle options:
     --burn-srt FILENAME
@@ -247,6 +252,7 @@ HERE
       @copy_audio                 = []
       @copy_audio_name            = []
       @aac_encoder                = nil
+      @reverse_double_order       = false
       @burn_subtitle              = nil
       @force_subtitle             = nil
       @extra_subtitle             = []
@@ -584,6 +590,8 @@ HERE
         end
       end

+      opts.on('--reverse-double-order') { @reverse_double_order = true }
+
       opts.on('--no-audio')           { force_handbrake_option 'audio', 'none' }

       opts.on '--burn-subtitle ARG' do |arg|
@@ -1081,6 +1089,15 @@ HERE
         bitrates << ''
       end

+      add_wide = ->(info, copy, ac3_first) do
+        if ac3_first
+          add_surround.call info, copy
+          add_stereo.call info, false
+        else
+          add_stereo.call info, false
+          add_surround.call info, copy
+        end
+
       track_order.each do |track|
         tracks << track
         info = media.info[:audio][track]
@@ -1109,11 +1126,9 @@ HERE
             names << name

             if @format == :mkv
-              add_surround.call info, copy
-              add_stereo.call info, false
+              add_wide.call info, copy, !@reverse_double_order
             else
-              add_stereo.call info, false
-              add_surround.call info, copy
+              add_wide.call info, copy, @reverse_double_order
             end
           else
             add_stereo.call info, copy

[lisamelton]

@samhutchins That's closer to what I had in mind. :)

But add_wide? That would be add_double if I were to do it that way. But I'm not sure that's how I would do it yet. I still haven't had my coffee. :)

[samhutchins]

Naming is hard man

Would you have it all inline? Like this?

if @format == :mkv
  if !@reverse_double_order
    add_surround.call info, copy
    add_stereo.call info, false
  else
    add_stereo.call info, false
    add_surround.call info, copy
  end
else
  if !@reverse_double_order
    add_stereo.call info, false
    add_surround.call info, copy
  else
    add_surround.call info, copy
    add_stereo.call info, false
  end 
end

[samhutchins]

I've reopened with a more thought out solution. In the event you take this pull request I think you can squash and merge to not have my silly first implementation in your history

[lisamelton]

@samhutchins Sorry I took so long to reply! I had my coffee hours and hours ago but that pesky family of mine is a distraction on holidays. :)

Thanks for the quick update(s) to your patch!

You found the right area to make the change now but your help text and code are too complicated. Here's a much simpler patch, and what I plan to do:

diff --git a/bin/transcode-video b/bin/transcode-video
index d6e5e58..8306f50 100755
--- a/bin/transcode-video
+++ b/bin/transcode-video
@@ -126,6 +126,8 @@ Audio options:
                       with `surround` to allow single surround or stereo track
                       with `stereo` to allow only single stereo track
                       (can be used multiple times)
+    --reverse-double-order
+                    reverse order of `double` "width" audio output tracks
     --prefer-ac3    prefer AC-3 over AAC format when encoding or copying audio
                       even when original track channel layout is stereo or mono
                       (sets audio output "width" for all tracks to `surround`)
@@ -241,6 +243,7 @@ HERE
       @audio_name                 = {}
       @audio_language             = []
       @audio_width                = {:main => :double, :other => :stereo}
+      @reverse_double_order       = false
       @prefer_ac3                 = false
       @ac3_bitrate                = 640
       @pass_ac3_bitrate           = 640
@@ -532,6 +535,8 @@ HERE
         end
       end

+      opts.on('--reverse-double-order') { @reverse_double_order = true }
+
       opts.on '--prefer-ac3' do
         @prefer_ac3 = true
         @audio_width[:main] = :surround
@@ -1108,7 +1113,7 @@ HERE
             tracks << track
             names << name

-            if @format == :mkv
+            if @format == :mkv ? !@reverse_double_order : @reverse_double_order
               add_surround.call info, copy
               add_stereo.call info, false
             else

Notice that I'm placing the new help text after --audio-width instead of nearer the end of the audio options. That's because users tend to read option descriptions in order and I want the terms "double" and "width" to still be in their heads. That way we can also avoid four lines of new help text. A single line should work fine now.

And my code change is just a single line. You had the logic right but we don't need a new nested function, nor do we need to repeat all that other code. This is much easier to understand and validate.

I'm actually away from my main development machine right now, but I should be able to land this in a few days.

Thanks again for kicking my ass to get this done. I should have known that sending the idea to you and @JMoVS in an email would prompt a patch. :)

[samhutchins]

This is much easier to understand and validate.

If you know Ruby syntax, sure :-P. Would you mind explaining the change to the logic there, I can't make sense of it at all (I'm a Java developer by trade, this some of the first Ruby I've written)

I think that the ? : is a ternary operator, but I don't understand what happens when Ruby evaluates the 2 statements around the :, and how they relate to the if statement.

Help text looks good, although I would say that you need to know the default order to understand what it does, that's why I included that in my help text

All that said, I just want the feature, don't mind too much how it works. Shall I update my pull request with your changes, or just will you just make the change?

[lisamelton]

@samhutchins Yes,? : is the Ruby ternary conditional operator. And it works pretty much the same as it does in C but you don't have to enclose the statement in parentheses. So, Ruby evaluates the whole statement to get the boolean value before it's passed to if.

Terseness is, for good or ill, a typical idiom in Ruby. :)

You can change your patch to match and I can squash and merge it, or I can land my patch on my own. It doesn't matter all that much. Whatever I do, it probably won't happen before Tuesday.

[samhutchins]

The parentheses make all the difference, I follow the logic now.

Whether or not that's easier to read, understand, and maintain is probably down to preference, but for me that terseness is horrible.

I'll update my pull request when I get a chance, gotta go count in the new year now though

[lisamelton]

@samhutchins Me too! Happy New Year, sir!

(And I'll consider adding parentheses. Two extra bytes probably won't kill us. :)

[samhutchins]

@donmelton I've updated the pull request with your patch

I chose to not use parens on the if statement because it doesn't match the style of the rest of the codebase, and I would say that consistent style is more important to overall readability. While the parens helped me to to see the logic, I don't think they make the code any more readable once you're in the Ruby headspace.

[lisamelton]

@samhutchins OK, I squashed and merged the change. Sorry it took so long but I'm ill at the moment. I'm not sure when I'll do the release.

[platonium]

Dear Don,

Hope you feel well soon! It’s been a nasty cold and flu season.

Best, Plato

P.S. if you don’t mind, could you let me know if you use any BluRay player on your Mac? It seems like it would be easier than running the BD back and forth which is what I do now.

Sent from my iPhone

On Jan 3, 2018, at 1:10 PM, Don Melton notifications@github.com<mailto:notifications@github.com> wrote:

@samhutchinshttps://github.com/samhutchins OK, I squashed and merged the change. Sorry it took so long but I'm ill at the moment. I'm not sure when I'll do the release.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/donmelton/video_transcoding/pull/184#issuecomment-355128014, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AOepbZyryYQvNYoJeXAfikhu58hbccmrks5tG-y3gaJpZM4RP2qY.

[lisamelton]

@platonium Thanks, I just wish it was a cold that I had. :)

On the rare occasion that I do play a Blu-ray Disc on my Mac (rather than just ripping it), I use the Macgo Mac Blu-ray Player Pro with the disc in my LG BE14NU40 drive.

[samhutchins]

@donmelton Take it easy man, hope you feel better soon

[lisamelton]

@samhutchins OK, I just released version 0.19.0 with this feature. Thanks, again!