Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for yuv[a]p16 pixel format and refactor video.frame.to/from_ndarray. #1722

Closed
wants to merge 4 commits into from

Conversation

robinechuca
Copy link
Contributor

  1. In order to handle yuv-to-rgb conversions yourself, it may be useful to convert frames to yuv444p16le before processing them in numpy. As described in a discussion.
  2. The to_ndarray and from_ndarray functions now use hash tables rather than a test tree. Complexity is therefore reduced from O(n) to O(1).
  3. More flexibility on the presence of a third channel for gray images.
  4. The automatic convertion between rgb and gbr is now optional

@WyattBlue WyattBlue closed this Jan 20, 2025
@WyattBlue
Copy link
Member

  1. Changing the numpy array dimensions of an existing test is an (undocumented!) breaking change and not allowed for our current release target (minor bump).

  2. Sets and hashmaps are more expensive to setup than tuples, negating any O(n) speed up when n is small and fixed.

@robinechuca
Copy link
Contributor Author

  1. For the first point, you're right, I'll fix it by adding a parameter so as not to break retrocompatibility.
  2. Regarding the second point, your argument is valid for very small assemblies. When there are more than 3 elements, creating a hash table is faster in the end (in interpreted python, in cython, I don't know).
timeit.timeit(lambda: "yuv420p" in {"yuv420p10le", "yuv420p10be", "rgb24"}, number=10000000)

Right now, there are more than 3 elements to compare. We can do a test to get an objective measure in this situation.

Assuming I correct the first point and convince you of the second, would you be willing to reconsider this pull request?

@WyattBlue
Copy link
Member

You're actually right for normal python (set is faster even for 1 element), but it might be a wash for Cython.

tuple 0.456623415928334
set 0.3176567500922829
difference: 43.75%
  5           RESUME                   0

  6           LOAD_CONST               1 ('yuv420p')
              LOAD_CONST               2 (('yuv420p10le', 'yuv420p10be', 'rgb24'))
              CONTAINS_OP              0
              RETURN_VALUE
None
  8           RESUME                   0

  9           LOAD_CONST               1 ('yuv420p')
              LOAD_CONST               2 (frozenset({'rgb24', 'yuv420p10le', 'yuv420p10be'}))
              CONTAINS_OP              0
              RETURN_VALUE
None

I guess using sets is okay for this PR

@robinechuca
Copy link
Contributor Author

Excellent idea to use frozenset instead of dynamic set! It is 11pm at home, I'll see it tomorrow.

@WyattBlue
Copy link
Member

That would be slower? Here's my full test code:

from timeit import timeit
import dis


def tuple_test():
    return 7 in (3, 6, 9)

def set_test():
    return 7 in {3, 6, 9}

def frozenset_test():
    return 7 in frozenset({3, 6, 9})

def main():
    tupt = timeit(tuple_test, number=10000000)
    sett = timeit(set_test,   number=10000000)
    sett2 = timeit(frozenset_test, number=10000000)
    
    print("tuple", tupt)
    print("set", sett)
    print("frozen set", sett2)
    print(f"difference: {round((tupt - sett) / sett, 4) * 100}%")

    print(dis.dis(tuple_test))
    print(dis.dis(set_test))
    print(dis.dis(frozenset_test))

if __name__ == "__main__":
    main()
tuple 0.473395915934816
set 0.33393541700206697
frozen set 1.2825716249644756
difference: 41.760000000000005%
  5           RESUME                   0

  6           LOAD_CONST               1 (7)
              LOAD_CONST               2 ((3, 6, 9))
              CONTAINS_OP              0
              RETURN_VALUE
None
  8           RESUME                   0

  9           LOAD_CONST               1 (7)
              LOAD_CONST               2 (frozenset({9, 3, 6}))
              CONTAINS_OP              0
              RETURN_VALUE
None
 11           RESUME                   0

 12           LOAD_CONST               1 (7)
              LOAD_GLOBAL              1 (frozenset + NULL)
              BUILD_SET                0
              LOAD_CONST               2 (frozenset({9, 3, 6}))
              SET_UPDATE               1
              CALL                     1
              CONTAINS_OP              0
              RETURN_VALUE
None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants