Fix segfault when accessing FilterContext after Graph object has left scope

[coredumped67]

Hello. I am experiencing a segfault when performing filter operations due to libav accessing an invalid Graph object. See the following example:

def make_filter(stream1: VideoStream, stream2: VideoStream) -> tuple[FilterContext, FilterContext, FilterContext]:
    graph = Graph()
    filter_source1 = graph.add_buffer(stream1)
    filter_source2 = graph.add_buffer(stream2)

    alphamerge = graph.add('alphamerge')
    filter_source1.link_to(alphamerge, 0, 0)
    filter_source2.link_to(alphamerge, 0, 1)

    filter_sink = graph.add('buffersink')
    alphamerge.link_to(filter_sink)

    graph.configure()
    return filter_source1, filter_source2, filter_sink
    # graph loses reference and gets free'd


if __name__ == '__main__':
    stream1, stream2 = get_streams()

    source1, source2, sink = make_filter(stream1, stream2)

    # Sources and sinks contain a weakref to the graph, and the
    # validity of said graph isn't properly checked during operations
    # such as FilterContext.push(...), resulting in a segfault.
    do_filter_stuff(source1, source2, sink)

My solution is to remove the weakref entirely, but I'm not sure how much of an impact this has on memory usage. I'm a little in over my head poking around in FFMPEG source, so I'm open to suggestions if this approach isn't feasible.

[WyattBlue]

This PR might bring regression for a memory leak problem. #1439

[coredumped67]

Hmmm. We could keep the weakref and check it's validity every time we cross the libav boundary. Users could still experience weird lifetime problems, but at least those would result in an exception rather than a segfault. Perhaps a "better" solution would be to perform all filter operations on the graph object itself. Something like this:

def make_filter(stream: VideoStream) -> Graph:
    graph = Graph()
    graph.add_buffer(stream, 'my-source')
    graph.add('crop', 'crop-to-thumbnail', '100:100')
    graph.link_to('my-source', 'crop-to-thumbnail', 0, 0)
    graph.add('buffersink', 'my-sink')
    graph.link_to('crop-to-thumbnail', 'my-sink', 0, 0)
    return graph
    
filter = make_filter(stream)
filter.push('my-source', frame)

I don't think the implementation would have to change too much. FilterContext would no longer be exposed and Graph would maintain a mapping of filter names to objects. But this approach would result in breaking changes :(

[WyattBlue]

Breaking changes are okay if you think that is the best solution.

[coredumped67]

Gotcha. If we were to go down the route of introducing breaking changes, would that come with a deprecation period to phase out the old interface?

[WyattBlue]

Yes, it will come with a depreciation period, but that's just out of scope when the new API hasn't been designed yet.

[coredumped67]

I've written a very basic PoC for my vision of what this tweaked interface should look like. I've not written tests yet, as I want to hear your thoughts before proceeding further.

[WyattBlue]

I think name now being a required argument is too great of a change.

[coredumped67]

I'm with you on that, but I can't think of another way we can safely keep the back reference as a weakref without breaking the API. If maintaining API stability is the no. 1 priority then I think we should take the less elegant route of checking weakref validity before crossing the libav boundary. The reference could still get garbage collected, but at least the library wouldn't segfault.