Page MenuHome

VSE: New iterator design
ClosedPublic

Authored by Richard Antalik (ISS) on Feb 5 2021, 9:15 PM.

Details

Summary

This iterator design provides means to create simple and flexible API
to query and work with collection of strips. It should be used in cases
when conditions require multiple stages of recursive iteration of all
strips or similar complex scenarios.

Quick API overview:
Basic queries are standalone functions that return SeqCollection
Use SEQ_collection_create() and SEQ_collection_free() to construct
such query functions.
Use these functions to get strips of known conditions, like selected
strips, movie strips, muted strips and so on.

Use SEQ_reference_query() when querying strips with relation to
some reference strip. For example to get its effects, strips that have
same type or use same input file and so on.
These aren't standalone functions because often you need to query strips
relative to each strip in collection.

Use SEQ_collection_expand() to query strips relative to each strip
in collection. These will be merged to original collection.

Use SEQ_collection_merge() to merge 2 collections

To iterate collection elements use macro SEQ_ITERATOR_FOREACH()

This API is quite specific, but I think it is best suited for tasks
that are usualy solved in sequencer codebase.

Old sequencer iterator has been completely removed.
SEQ_ALL_BEGIN and SEQ_ALL_END macros re-use new iterator design.

As initial use for this iterator select_grouped_effect_link()
function has been rewritten. It was not only broken, but also it used
DNA fields to aid iterating strips.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D10337 (branched from master)
Build Status
Buildable 13730
Build 13730: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Feb 5 2021, 9:15 PM
Richard Antalik (ISS) created this revision.
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Feb 5 2021, 9:20 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Feb 5 2021, 9:35 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • Update design to more polished version
  • Minor fixes + style
Richard Antalik (ISS) retitled this revision from Sequencer Iterator design to VSE: New iterator design.Mar 26 2021, 11:19 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)

Is a bit hard to dig into details of examples from the description, but overall idea of using GSet and some utility functions seems totally fine for me. How exactly to organize them you'd know better.

The API is confusing to me:

  • What the difference between SEQ_iterator_reference_query and SEQ_iterator_query is?
  • It feels they do too much: run callback and return iterator data. Semantically, iterator is something what lets you to iterate over elements of a collection; query is something which queries (possibly, with filter) elements from the collection. Query can either use callback-style communication with the caller, or provide an iterator, so that caller can iterate over results. Having both at the same time is confusing.
  • SEQ_iterator_expand -> SEQ_iterator_run_foreach ? Why does it need both seqbase and iterator? This again feels that this function maintains the iterator and invokes the callback.

Quite sure it's possible to streamline the API a lot and make it easy to follow.

Is a bit hard to dig into details of examples from the description, but overall idea of using GSet and some utility functions seems totally fine for me. How exactly to organize them you'd know better.

The API is confusing to me:

  • What the difference between SEQ_iterator_reference_query and SEQ_iterator_query is?

Whole idea with reference is to get collection based on reference strip. Like effects of this strip or strip using this strip. Otherwise SEQ_iterator_query doesn't require reference, for example all selected strips or all color strips...
Originally it was one function, but I had to pass NULL when no reference was needed - not very nice.
Right now it's not much better though.

  • It feels they do too much: run callback and return iterator data. Semantically, iterator is something what lets you to iterate over elements of a collection; query is something which queries (possibly, with filter) elements from the collection. Query can either use callback-style communication with the caller, or provide an iterator, so that caller can iterate over results. Having both at the same time is confusing.

This is partially semantic issue, partially unfinished design. Query should return collection, iterator there is for convinience. It can and probably should be implemented in macro though. So effectively it is created by caller

Quite sure it's possible to streamline the API a lot and make it easy to follow.

Would it be possible to write this in C++ and use optional arguments to resolve issue with queries and additional data like reference strip ans possibly filters? Though I would rather do filters more explicitly.
There will be probably other advantages of C++ but I haven't used that language ever so I would need to read about it's features. Probably OOP would be better suited for this job in general?

  • Completely remove old iterator and make basic queries more independent of API. I am still unhappy about this layout...
  • Rename SeqQueryResult to SeqCollection
  • Add docs
  • Merge branch 'master' into arcpatch-D10337_1
Richard Antalik (ISS) planned changes to this revision.May 6 2021, 7:48 AM

I have found out that SEQ_ITERATOR_FOREACH macro needs unique varieble name or it needs to be split in begin/end in same way as gset iterator is.
Also SEQ_ALL_BEGIN / SEQ_ALL_END could rather reuse gset iterator, it would be cleaner

Richard Antalik (ISS) edited the summary of this revision. (Show Details)May 6 2021, 8:47 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • Hide SEQ_ITERATOR_FOREACH iterator variable from scope of function body. Now macro user needs to explicitly declare Sequence variable that has to be declared in scope outside of iterator.

@Sergey Sharybin (sergey) Can you review this patch? I think the API is best possible fit for VSE needs. Some subtle changes can be made on the fly.

There are some inline comments which should be easy to address. The naming about collection one is a bit tricky, so maybe leave it as-is for now.

Guess at this point is better to address the inlines, get the change committed, and re-iterate on it if/when needed.

source/blender/sequencer/SEQ_iterator.h
68

I don't see SeqQueryResult anywhere. Did you miss some occurrences during rename?

83
source/blender/sequencer/intern/iterator.c
102

I'm a bit unsure about use of Collection. Mathematically it is correct term, but somewhat conflicts with Blender's Collections.
But can't think of a better alternative at this time :(

108

Maybe SEQ_query_by_reference() ?

Richard Antalik (ISS) marked 2 inline comments as done.
  • Address inlines.
source/blender/sequencer/SEQ_iterator.h
68

Yes that was name I used for SeqCollection originally, which I can use if you don't like SeqCollection...

I decided against SeqQueryResult because prefix for functions would be SEQ_query_result_xxx and that may imply that function is query function.

Richard Antalik (ISS) marked an inline comment as done.May 7 2021, 9:30 AM

There are couple of more of non-functional-changes you can do, but can also happen right before push (as in, no need in the review iteration).

I think we should accept the SeqCollection. It is semantically correct, and for until someone comes with a strong proposal and motivation to change, we just live with it.
I really like how the use of seq->tmp is being removed with this change! :)

source/blender/sequencer/SEQ_iterator.h
68

Well, you query something and put it into a collection type of a storage. Semantically it is clear.
Just feels weird about possible perception of conflicting terminology within Blender.

But again, don't have suggestion which is strongly better than the current code.

source/blender/sequencer/intern/iterator.c
104
106
This revision is now accepted and ready to land.May 7 2021, 9:50 AM
This revision was automatically updated to reflect the committed changes.