Add color_range to CodecContext/Frame

[johanjeppsson]

Hi!

I recently found PyAV, and I am very impressed with how flexible and powerful it is compared to my old go-to-solution, which was calling ffmpeg with a subprocess call! :)

I realized that the color_range option was not implemented so I gave it a shot. Specifically, this can be needed when the default color range for YUV (0-255 or 16-235) needs to be overridden for a codec.

Since I am not deeply familiar with PyAV or the FFmpeg source code, my approach might not be the way to go, and if so I'm happy to update it based on your suggestions. In particular, the change in reformatter.pyx doesn't seem like the right solution to me. As far as I can tell it works as intended, but it definitely has a smell to it...

Also, some tests should probably also be added for this. I don't know if it's enough to test that the color_range property is set as expected when provided as an option, or if some decoding with different color_ranges are preferable?

[jlaine]

@mikeboers I am not familiar with this, any thoughts?

[mikeboers]

I like the idea.

I'm intrigued by the method of passing the current range to the reformatter by respecting the frame. But we already have {src,dst}_colorspace being passed to the reformatter, so I'm not keen on having two methods of specifying different aspects of the color transform.

Casually looking at it, this block feels like it will have lots of weird state issues.

I think it would be cool to:

• Have colorspace and color_range properties on a frame.
• Have the reformatter have {src,dst}_{colorspace,color_range} params.
• Have the src params take default values from the frame if None.

[jlaine]

By the way, could you rebase your changes, it should give you a clean CI baseline?

[johanjeppsson]

By the way, could you rebase your changes, it should give you a clean CI baseline?

I think that it should be rebased now.

I like the idea.

I'm intrigued by the method of passing the current range to the reformatter by respecting the frame. But we already have {src,dst}_colorspace being passed to the reformatter, so I'm not keen on having two methods of specifying different aspects of the color transform.

Casually looking at it, this block feels like it will have lots of weird state issues.

I think it would be cool to:

* Have `colorspace` and `color_range` properties on a frame.

* Have the reformatter have `{src,dst}_{colorspace,color_range}` params.

* Have the src params take default values from the frame if None.

Your suggestion sound good, but I feel that I don't have a full grasp all the implications that might come from changing this section. I'll try to have look at it later (hopefully tomorrow) when I have a bit more time on my hands.

[johanjeppsson]

I like the idea.

I'm intrigued by the method of passing the current range to the reformatter by respecting the frame. But we already have {src,dst}_colorspace being passed to the reformatter, so I'm not keen on having two methods of specifying different aspects of the color transform.

Casually looking at it, this block feels like it will have lots of weird state issues.

I think it would be cool to:

* Have `colorspace` and `color_range` properties on a frame.

* Have the reformatter have `{src,dst}_{colorspace,color_range}` params.

* Have the src params take default values from the frame if None.

I've given your suggestion a shot. Have a look and let me know what you think!

[johanjeppsson]

Should I do anything else here?

[jlaine]

@mikeboers as I really have no insights into this could you just let me know if this is good to merge?

[riaqn]

I wonder what's the status on this PR? really useful to me...

[mfoglio]

Is this feature available in PyAv 8.0.3?

input_container = av.open(url)
input_container.streams.video[0].codec_context.color_range

Traceback (most recent call last):
  File "/home/ubuntu/.pycharm_helpers/pydev/_pydevd_bundle/pydevd_exec2.py", line 3, in Exec
    exec(exp, global_vars, local_vars)
  File "<input>", line 1, in <module>
AttributeError: 'av.video.codeccontext.VideoCodecContext' object has no attribute 'color_range'

[jlaine]

Unless @mikeboers objects, my only additional request here would be to have some unit tests which exercise:

• the new getters / setters
• the additional arguments to reformat

[mikeboers]

I'm game. 👍

[vade]

Curious on the status of this PR, as this functionality is almost mandatory on professional videos with colorspaces outside of the standard rec.709 assumptions.

Thank you.

[djha-skin]

@mikeboers @jlaine I would like to offer my services if there is anything else needed to get this PR across the finish line.

I am very grateful for the existence of this library. It has helped immensely at my place of work in doing what we do. This PR would really allow add that extra polish to something I'm working on at work.

You should let me help. What can I do?

[hmaarrfk]

As an outsider (but also user of this library) I think that generally the norm in the open source community is to:

1. Attempt to fork off @johanjeppsson work
2. Maintain their git commit history (rebase or merge)
3. Build commits onto of it to address @jlaine's requests for light testing

[jlaine]

@WyattBlue I think something went badly wrong with your git manipulation, there are now ten of commits in this PR?

[WyattBlue]

@jlaine I think I reverted those dummy commits. Didn't know merging in a fork would edit the PR.