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

H264Libav: Handle colour space conversion #1814

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

any1
Copy link
Contributor

@any1 any1 commented Aug 19, 2024

The scaler is now informed of the colour space encoded into the stream so that it may do the proper conversion.

The scaler is now informed of the colour space encoded into the stream
so that it may do the proper conversion.
@cgm999
Copy link

cgm999 commented Aug 21, 2024

@any1 For those that are not knowing much about ffmpeg what are the benefits of the patch?

@any1
Copy link
Contributor Author

any1 commented Aug 21, 2024

The H.264 Annex B bit stream contains information about the YCbCr conversion coefficients, range, colour primaries and transfer characteristics. This passes that information on to the scaler so that it can convert the input colours so that they use the same colour profile as the RGB buffer into which they're rendered.

The parameter that is most important here and will be most obvious when it's wrong is colour range. H.264 is most often encoded using limited colour range, which means that values range from 16 to 235. This is done for no good reason other than "that's how it's always been done", but let's not dwell on it.

If the input has limited range, but you don't map it to the full 0 - 255 range when rendering it, the colours will not be quite right. The same applies, but in a different way if you do map something that's already full range to full range.

@cgm999
Copy link

cgm999 commented Aug 21, 2024

@any1 Interesting, TYVM for your detalied explanation .

@cgm999
Copy link

cgm999 commented Aug 22, 2024

Hi @any1, since you are already doing changes related to sws_scaling, can you also set that flag to SWS_BICUBIC :
sws = sws_getCachedContext(sws, frame->width, frame->height, avctx->pix_fmt,
frame->width, frame->height, AV_PIX_FMT_RGB32,
SWS_BICUBIC, nullptr, nullptr, nullptr);
without it it will not cache the sws object.. and return a new one every time (easy to see ,add below line after sws_getCachedContext )
vlog.error("sws=%p\n",sws);

I did tested this PR on my pikvm .. can not see issues with it .

@any1
Copy link
Contributor Author

any1 commented Aug 22, 2024

Sure, I can put a commit in there to fix this, but I'd use SWS_POINT as there is no actual scaling taking place. Using any form of interpolation is just a waste of resources.

@cgm999
Copy link

cgm999 commented Aug 22, 2024

Agreed, I tested with SWS_POINT and it works fine.

sws_getCachedContext will set a default sampling method if 0 is passed
to the flags argument. This means that when it is called again, the
flags argument will not match the flags in the context, so a new one
will be allocated every time.

To get around this problem, we assign an explicit sampling method, one
that also happens to be more efficient and just as good for this
use-case as the default one, which is bicubic interpolation.
@any1
Copy link
Contributor Author

any1 commented Aug 22, 2024

Added the commit for the sampling method.

@cgm999
Copy link

cgm999 commented Aug 23, 2024

Thank you @any1 .
My testing (with pikvm as H264 source and 2 diff systems connected to it, changed resolutions ) did not show any issues with both commits applied.

@CendioOssman CendioOssman merged commit cfed31b into TigerVNC:master Aug 23, 2024
11 checks passed
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.

3 participants