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

Optimise decklink producer video filtering #1253

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

scriptorian
Copy link
Contributor

Comparing V2.1 and V2.2 decklink producers there is a lot more CPU processing required by FFmpeg filters in V2.2. Interlaced sources are now always deinterlaced but in addition there are colorspace and fps filters added. In addition the filter output pixel format is set to BGRA which requires a CPU-based color conversion when this conversion is available on the GPU as part of the mixer.

This pull request optimises these extra filters to reduce CPU load as follows:

  1. Filter output pixel format is now YUV422P
  2. Remove the colorspace filter - now done by the mixer input processing
  3. Only insert the fps filter if the decklink framerate doesn't match the channel
  4. Use a lower quality deinterlace if using an interlaced input to a channel set to an interlaced format

Early testing showed a 30% reduction in CPU load on a 6-channel setup.

Comments welcome.

@premultiply
Copy link

premultiply commented Mar 4, 2020

Where is the correct colorspace conversion code?
There is a good reason why RGBA as sample format and sRGB is chosen as colorspace as uniform mixer format as the required conversion matrix depends on the input format.

Server 2.1 was simply wrong on this.

@Julusian
Copy link
Member

Julusian commented Mar 4, 2020

the required conversion matrix depends on the input format

The mixer was already coded to account for rec709 (or the sd equivalent) when converting ycbcr to rgb, so this is leaving it to be done there rather than via ffmpeg.
https://github.com/CasparCG/server/blob/master/src/accelerator/ogl/image/shader.frag#L461

@premultiply
Copy link

Ok, but how does the mixer decide about the color matrix to use?

@scriptorian
Copy link
Contributor Author

The mixer decides which color matrix to use based on an sd/hd swiitch set from the channel image height.

@premultiply
Copy link

Ok, but that might lead to problems for 1080p50 and higher resolutions as they can be BT.709 or BT.2020.

@Julusian
Copy link
Member

Julusian commented Mar 5, 2020

If/when that is a problem then it would be simple to inform the mixer of what to use rather than leaving it guess.
But as there wasnt any support for BT.2020 yet in the decklink producer, this isnt breaking anything on that.

@premultiply
Copy link

I would prefer to add a colormatrix parameter to the mixerright now instead of guessing only on picture height.
Should be more clean from my point of view.
Stuff like this lead to too many issues in the past.

@Julusian
Copy link
Member

Julusian commented Mar 5, 2020

Yes, I agree that a parameter would be a good idea. Should be pretty quick to add an auto/sd/hd enum on whatever pix_desc is for that.
What do you think @scriptorian?

@scriptorian
Copy link
Contributor Author

I agree it would be much better to handle colorspace correctly but there are a few issues:

  1. The pix_desc doesn't contain any colorspace information
  2. The pix_desc is created from an AVFrame but the creation doesn't read any colorspace information from the AVFrame
  3. That's OK because the producer doesn't actually set up any colorspace information in the AVFrame

All of this is addressable but I imagine any changes ought to propagate to all producers.

On a more pedantic point, should we worry about the fact that the current code is not taking any account of the differing chromaticity coordinates and potential differing gamma of the various standards? BT.2020 in particular includes a wider color gamut which means that more processing is required to correctly handle it when mixing with other standards. The other outlier is sRGB which has a quite different gamma curve and does not seem to be handled in the code currently.

I would have some concerns about trying to address these issues in an 8-bit processing pipeline and fixing all this systematically will consume more CPU and or GPU cycles and may lead to some 8-bit processing artefacts that are not visible right now. Then we just have to worry about HDR!

Since the existing code doesn't handle 2020 either I propose that these proposed changes should be carefully written up as a new issue. Thoughts?

@premultiply
Copy link

premultiply commented Mar 5, 2020

Well, in the meantine I thought a littlebit about it.

As CasparCG currently does only support SDR and all mixing is done in 8-bit sRGB (and „full range“) only conversion from/to BT.709 and BT.601/BT.470 needs to be supported. For WCG and HDR much more things need to be changed so I think this should be out of the focus for now.

sRGB and BT.709 share at least the same primary chromaticities. Other charcteristics are at least simillar so I think it should be ok for now.

Maybe we should have a look at it at a later time again to make shure it is accurate enough.
Important is that conversion to and from internal sRGB lead to nearly the same result.

@premultiply
Copy link

premultiply commented Mar 9, 2020

What is an issue is the different gamma curve between BT.709 and sRGB.

Well I think sooner or later it is
nessesary to switch the mixer to linear RGBA (no gamma) and internal 16 bit float operations.
There should be mapping/conversion tools availible in OpenGL.

You can find some informations here https://stackoverflow.com/questions/6397817/color-spaces-gamma-and-image-enhancement

@Julusian
Copy link
Member

Julusian commented Mar 9, 2020

@premultiply we did internally briefly discuss larger changes like that to the mixer, but it was decided to not do it yet

@scriptorian Ah, I did not realise we were creating it from an AVFrame via some abstractions. That does make it fiddlier than I was hoping. Lets leave it like this for now then as it should be accurate, and it can be revisited with larger work on the mixer

@Julusian Julusian merged commit c93b5de into CasparCG:master Mar 30, 2020
@dotarmin dotarmin added this to the v2.3.0 LTS milestone Apr 7, 2020
@scriptorian scriptorian deleted the decklink_producer_opt branch June 24, 2020 16:55
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.

4 participants