Stream selectors, `.map` operator (audio support) by depau · Pull Request #45 · kkroening/ffmpeg-python

depau · 2017-12-21T16:51:32Z

Implements stream selection and mapping multiple streams to one output.

Example:

i = ffmpeg.input("video_with_sound.mp4")
audio = i[:"a"].filter_("aecho", 0.8, 0.9, 1000, 0.3)
video = i[:"v"].hflip().vflip()
single_output = ffmpeg.output(video, audio, "my_output.mp4")
# or
single_output = video.output("my_output.mp4")
single_output = single_output.map(audio)

ffmpeg -i video_with_sound.mp4
-filter_complex
'[0:v]hflip[s0];[s0]vflip[s1];[0:a]aecho=0.8:0.9:1000:0.3[s2]'
-map '[s1]' -map '[s2]' my_output.mp4

Closes #26 and #42

kkroening · 2017-12-23T06:30:04Z

+
+    def _add_streams(self, stream_spec):
+        """Attach additional streams after the Node is initialized.
+        """


This is odd - Nodes should be considered immutable (see notes in dag.py). What's the intent of retroactively rewiring an already-created node?

I think I see now. Mapping can be done either with a multi-stream output or using .map, and .map takes an existing output node and makes it a multi-output node.

Can we just make a new output node with the desired streams rather than mutating the existing one?

Making nodes mutable has a lot of widespread implications and opens up the complexity to spiral out of control - for example, downstream hashes would need to be recursively recalculated. (See some of the benefits of immutable data-structures). I'd rather not make such a fundamental change unless absolutely necessary.

Can we just make a new output node with the desired streams rather than mutating the existing one?

I agree that this should be the best way to do it, I did it this way as I didn't know how to get back the stream specs from an existing node to regenerate the new OutputNode.

Do you have any ideas?

kkroening · 2017-12-23T06:45:19Z

+    i2 = ffmpeg.input(TEST_OVERLAY_FILE)
+
+    o_map = i1.output(TEST_OUTPUT_FILE1)
+    o_map.map(i2)


See comment above about immutability:

- o_map.map(i2) + o_map = o_map.map(i2)

kkroening · 2017-12-23T06:53:42Z

            outgoing_stream_type=OutputStream,
-            min_inputs=1,
-            max_inputs=1,
+            min_inputs=0,  # Allow streams to be mapped afterwards


I don't think this is necessary. Tests pass with this change removed. See also comment about immutability.

kkroening · 2017-12-23T06:54:22Z

+# self.__check_input_types(stream_map, incoming_stream_types)
+# incoming_edge_map = self.__get_incoming_edge_map(stream_map)
+# super(Node, self).__init__(incoming_edge_map, name, args, kwargs)
+# self.__outgoing_stream_type = outgoing_stream_type


Remove commented code.

Done 4349b18

kkroening · 2017-12-23T06:59:28Z

    outgoing_edges = get_outgoing_edges(node, outgoing_edge_map)
-    inputs = [stream_name_map[edge.upstream_node, edge.upstream_label] for edge in incoming_edges]
-    outputs = [stream_name_map[edge.upstream_node, edge.upstream_label] for edge in outgoing_edges]
+    inputs = ["[{}{}]".format(stream_name_map[edge.upstream_node, edge.upstream_label], "" if not edge.upstream_selector else ":{}".format(edge.upstream_selector)) for edge in incoming_edges]


This line has gotten too complicated and is basically unreadable. Can we split it out a bit?
e.g.

def _format_input_stream_name(stream_name_map, edge): prefix = stream_name_map[edge.upstream_node, edge.upstream_label] if not edge.upstream_selector suffix = "" else: suffix = ":{}".format(edge.upstream_selector) return "[{}{}]".format(prefix, suffix) def _format_output_stream_name(stream_name_map, edge): return "[{}]".format(stream_name_map[edge.upstream_node, edge.upstream_label]) def _get_filter_spec(node, outgoing_edge_map, stream_name_map): incoming_edges = node.incoming_edges outgoing_edges = get_outgoing_edges(node, outgoing_edge_map) inputs = [format_input_stream_name(stream_name_map, edge) for edge in incoming_edges] outputs = [format_output_stream_name(stream_name_map, edge) for edge in outgoing_edges]

Done 21bd1cd

kkroening · 2017-12-23T07:04:27Z

+
+    for edge in node.incoming_edges:
+        # edge = node.incoming_edges[0]
+        stream_name = "[{}{}]".format(stream_name_map[edge.upstream_node, edge.upstream_label], "" if not edge.upstream_selector else ":{}".format(edge.upstream_selector))


This line is too complicated. See comment about _format_output_stream_name above.

i.e.

- stream_name = "[{}{}]".format(stream_name_map[edge.upstream_node, edge.upstream_label], "" if not edge.upstream_selector else ":{}".format(edge.upstream_selector)) + stream_name = _format_input_stream_name(stream_name_map, edge)

Done 21bd1cd

kkroening · 2017-12-23T07:07:35Z

    edges = []
-    for downstream_label, (upstream_node, upstream_label) in list(incoming_edge_map.items()):
-        edges += [DagEdge(downstream_node, downstream_label, upstream_node, upstream_label)]
+    # downstream_label, (upstream_node, upstream_label) in [(i[0], i[1][:2]) for i in self.incoming_edge_map.items()]


Remove commented line.

Done 4349b18

kkroening · 2017-12-23T07:08:21Z

-    for downstream_label, (upstream_node, upstream_label) in list(incoming_edge_map.items()):
-        edges += [DagEdge(downstream_node, downstream_label, upstream_node, upstream_label)]
+    # downstream_label, (upstream_node, upstream_label) in [(i[0], i[1][:2]) for i in self.incoming_edge_map.items()]
+    for downstream_label, upstream_info in [(i[0], i[1]) for i in incoming_edge_map.items()]:


This line is magical and overly complex. What do i[0] and i[1] mean? Is there an i[2]? Can we split this out into something that's more obvious to understand?

kkroening · 2017-12-23T07:10:19Z

+    # downstream_label, (upstream_node, upstream_label) in [(i[0], i[1][:2]) for i in self.incoming_edge_map.items()]
+    for downstream_label, upstream_info in [(i[0], i[1]) for i in incoming_edge_map.items()]:
+        upstream_node, upstream_label = upstream_info[:2]
+        upstream_selector = None if len(upstream_info) < 3 else upstream_info[2]


Where does the magical 3 constant come from? Needs explanation or named constant.

e.g. at the top of the file

_MEANINGFULLY_NAMED_CONSTANT = 3

then

- upstream_selector = None if len(upstream_info) < 3 else upstream_info[2] + upstream_selector = None if len(upstream_info) < _MEANINGFULLY_NAMED_CONSTANT else upstream_info[2]

or split it out to make it more clear:

if len(unsptream_info) < 3: # explanation goes here upstream_selector = None else: upstream_selector = upstream_info[2]

kkroening · 2017-12-23T07:15:40Z

-        for (downstream_node, downstream_label) in downstream_infos:
-            edges += [DagEdge(downstream_node, downstream_label, upstream_node, upstream_label)]
+        for downstream_info in downstream_infos:
+            downstream_node, downstream_label = downstream_info[:2]


What's [:2] in this case? (need comment/explanation)

Done 14b6470

kkroening · 2017-12-23T07:16:53Z

        hashes = []
-        for downstream_label, (upstream_node, upstream_label) in list(self.incoming_edge_map.items()):
+        # This is needed to allow extra stuff in the incoming_edge_map's value tuples
+        for downstream_label, (upstream_node, upstream_label) in [(i[0], i[1][:2]) for i in self.incoming_edge_map.items()]:


What are i[0], i[1], etc.? Should make this code more clear and split it out into something easier to understand. Way too much happening in this line.

Done 14b6470

kkroening · 2017-12-23T07:17:25Z

    def __upstream_hashes(self):
        hashes = []
-        for downstream_label, (upstream_node, upstream_label) in list(self.incoming_edge_map.items()):
+        # This is needed to allow extra stuff in the incoming_edge_map's value tuples


What does "extra stuff" mean? Comment should be more clear I think.

Done 14b6470

kkroening · 2017-12-23T07:18:44Z


 def _get_stream_name(name):
-    return '[{}]'.format(name)
+    return '{}'.format(name)


Should just get rid of this function.

Done 03a20ff

kkroening

Overall good approach. The main thing is immutability - see these comments first. Then a bunch of other relatively minor things of just making the code easier to read.

Honestly I don't see why we need .map since it can all be done with .output (if I understand correctly), but I guess it's okay to have it as long as it doesn't break immutability. I would also be happy with just having .output though.

Anyways, overall nice work. Feel free to ping me for more discussion.

depau · 2017-12-23T15:22:49Z

I agree with all your comments. I will probably get to it after holidays. See my reply to your immutability comment.

depau · 2018-01-09T10:36:25Z

Okay, the syntactic part should be done. I'll do the immutability part now. Let's see what I come up with.

By the way, some tests are still failing because of py33. I think we can just let Travis alone until it's all done, then rebase the whole thing on top of master.

* Selectors are used just like 'split', i.e. `stream.split()[0:"audio"]`

* No need to have a split node in between, you can just do stream[:"a"] * Split nodes are still needed to do actual splitting.

…` and `view()`

…in Ubuntu's Python3

depau · 2018-01-09T15:11:06Z

What the hell happened? I think I screwed something up with rebase, I don't know how the "Update README" commits came into the PR.
Anyway, it should be done. Check the new immutable implementation.

…or now)

kkroening reviewed Dec 23, 2017

View reviewed changes

kkroening requested changes Dec 23, 2017

View reviewed changes

depau force-pushed the stream_selectors branch from 7fb7026 to 21bd1cd Compare January 9, 2018 10:03

noahstier and others added 13 commits January 9, 2018 15:47

Add 'crop' filter

fabd401

Update README.md

db89774

Update README.md

65a0682

Update README.md

614a558

Update README.md

f2a37d3

Update README.md

fb7fe24

Update README.md

fc86457

Update README.md

d904e15

Update README.md

e66a093

Update README.md

0e47a6a

Update README.md

be3f300

Update README.md

2a95d6f

Update README.md

63e4218

depau added 18 commits January 9, 2018 15:47

Implement selectors in Stream and Node

646a0dc

* Selectors are used just like 'split', i.e. `stream.split()[0:"audio"]`

Add __getitem__ to Stream too, simplify selector syntax

f6d0145

* No need to have a split node in between, you can just do stream[:"a"] * Split nodes are still needed to do actual splitting.

Take into account upstream selectors in topological sort, `get_args()…

3f67121

…` and `view()`

Make sure item is instance of slice in __getitem__

c2c6a86

Allow outputs to be created empty; streams can be mapped later

44091f8

Generate multiple -map for outputs with multiple incoming edges

aa0b0bb

Implement .map() operator, allow multiple streams in .output()

0d95d9b

Explicitly include -map [0] when output has multiple mapped streams

9065230

Add tests for stream selection and mapping

db83137

Allow output to be created without mapped streams

b4503a1

Expand complicated format + list comprehension into its own function

03762a5

Remove commented code

1070b3e

Remove useless _get_stream_name function

1a46471

Remove unused imports

783bdbd

Simplify, expand and explain complicated loops in dag.py

861980d

Reimplement .map logic making Node immutable

497105f

Replace past.builtins.basestring with a custom one to workaround bug …

df9bd73

…in Ubuntu's Python3

Expand unclear one-line implicit conditional statement

ea90d91

depau force-pushed the stream_selectors branch from 14b6470 to ea90d91 Compare January 9, 2018 15:05

Merge branch 'master' into stream_selectors

ef9b102

depau mentioned this pull request Feb 16, 2018

Add 'alsa' audio to /dev/videoX input #70

Closed

depau mentioned this pull request Mar 8, 2018

Support non-Python characters in node arguments #73

Closed

kkroening added 5 commits March 10, 2018 19:01

Merge remote-tracking branch 'origin/master' into stream_selectors

809ab6c

Minor improvements (formatting, etc)

6169b89

Change selector syntax from [:a] to [a]; remove map operator (f…

57abf6e

…or now)

Test bad stream selectors

1e63419

Update docs

c162eab

kkroening merged commit 8420f3b into kkroening:master May 9, 2018

juzsal mentioned this pull request Nov 11, 2018

AttributeError: 'FilterableStream' object has no attribute 'map' #139

Closed

Conversation

depau commented Dec 21, 2017

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

depau Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroening left a comment

Choose a reason for hiding this comment

Uh oh!

depau commented Dec 23, 2017

Uh oh!

depau commented Jan 9, 2018

Uh oh!

depau commented Jan 9, 2018

Uh oh!

kkroening Dec 23, 2017 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading

depau Jan 9, 2018 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading

kkroening Dec 23, 2017 •

edited

Loading