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

VideoPlayer: add method for requesting desired video resolution to de… #9531

Merged
merged 1 commit into from Apr 3, 2016

Conversation

FernetMenta
Copy link
Contributor

…muxer

@mapfau not complete finished but should to for now. I only sets resolution at the time video stream is opened. Can be improved later.

@FernetMenta FernetMenta added Type: Feature non-breaking change which adds functionality v17 Krypton Component: Video labels Apr 2, 2016
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<addon id="kodi.inputstream" version="1.0.3" provider-name="Team Kodi">

This comment was marked as spam.

This comment was marked as spam.

@ace20022
Copy link
Member

ace20022 commented Apr 2, 2016

Imo it would be better to use video steam selection, we do this for any other stream type.

@ghost
Copy link

ghost commented Apr 2, 2016

@ace20022 i thought we agreed for both? automatic and manual selection?

@ace20022
Copy link
Member

ace20022 commented Apr 2, 2016

The others have both too. The videoplayer chooses a preferred stream from all available and not the demuxer.

@FernetMenta
Copy link
Contributor Author

It's the demuxer that can change resolution and signal stream change event. Hence it is correct to instruct demuxer with desired resolution.

@FernetMenta
Copy link
Contributor Author

@ace20022 not sure if you got the intent of this change. DASH uses bandwidth to decide what of that many available streams is best. But on a 720 it won't make sense to select a 1080 stream. This is nothing that can go to selection streams. Destination resolution can change by i.e. resizing the window.

@ace20022
Copy link
Member

ace20022 commented Apr 2, 2016

@ace20022 not sure if you got the intent of this change.

ofc I got it.

But on a 720 it won't make sense to select a 1080 stream.

Why not? Downsampling for example?

Imo a demuxer should only offer streams/resources and should not act on its own.

@FernetMenta
Copy link
Contributor Author

Imo a demuxer should only offer streams/resources and should not act on its own.

heh? what about the "adaptive" in DASH? We already do this and this is just another parameter for optimization.

Why not? Downsampling for example?

Certainly not a good idea. Why do you want to waste resources on network on GPU? Downloading 1080 and downscaling it to 720 makes ZERO sense if you can download 720 right away.

@FernetMenta FernetMenta added this to the Krypton 17.0-alpha1 milestone Apr 2, 2016
@phate89
Copy link
Contributor

phate89 commented Apr 2, 2016

I can't comment on the code but a downsampled 1080p looks usually better than a native 720p.. Web streams use the minimum bit rate necessary for a decent quality so usually you get more details with the downsampled version..
Of course the providers doesn't handle this way because for them bandwidth is a cost so they give you the minimum they can...

@FernetMenta
Copy link
Contributor Author

@phate89 I don't think you fully understand the topic here. In order to optimize an addon is free to use various parameters like bandwidth, desired resolution or whatever else is necessary.
Sure, the quality may be batter when downscaling but not if player has to drop/skip frames because download is too slow. The more information an addon gets, the better it can optimize.
Again, this is about adaptive. An addon is free to to decide on its own what is best. We have stream change event for an addon to signal to player that stream parameters have changed. Player / renderer can adapt to changes on the fly without loss of frames.

@fritsch
Copy link
Member

fritsch commented Apr 2, 2016

@phate89 As I have seen this "myst" again and again here is a counter example:

1080p low quality (45 KB)
1080p-low-quality

720p mid quality (37 KB)
720p-mid-quality

1080p rescaled from above 720p (size does not matter as our lanczos3 does this)
1080p-rescaled-from-720p

Summary: When bandwidth gets really low I always prefer the 720p version at this bandwidth.

@FernetMenta
Copy link
Contributor Author

jenkins build this please

@ghost
Copy link

ghost commented Apr 2, 2016

I said already, that I'll make resolutions selectable if anybody wnats to play around with it.
90% ore more does'nt want to play around with it. Its an option to support adaptive Bitrate switching and I want to have it enabled in the best possible way. I'll let you choose whatever you wnt.

@phate89
Copy link
Contributor

phate89 commented Apr 3, 2016

@fritsch you are doing the opposite..
plus if you have low 1080p you have low 720p..Or at least you have a scalar progression of quality..
first youtube video I found 720p vs 1080p downsized with lanczos3:
http://screenshotcomparison.com/comparison/167861
That's from my experience with x264 encoding at least..
OF course if the user/addon is able to select it anyway @FernetMenta is right and my comment is pointless

@FernetMenta
Copy link
Contributor Author

@phate89 yes, the user is can still select the desired quality or an automatic mode. For now player requests window resolution but this will be more dynamic in the future. Not sure if you have noticed ProcessInfo. This object is supposed to collect runtime data and make many quality settings like deinterlacing of upscaling automatic. Means if there are enough system resources VideoPlayer may request the highest resolution and sets downscaling to the best quality. On a different scenario VideoPlayer infrastructure may be used to render into a preview window, PIP, or transcoding on-the-fly. Those are scenarios player has to decide on its own anyway.

jenkins build this please

@FernetMenta FernetMenta merged commit b46f99b into xbmc:master Apr 3, 2016
@FernetMenta FernetMenta deleted the inputstream branch April 3, 2016 09:44
@ghost
Copy link

ghost commented Apr 3, 2016

thanx! will implement soon (currently its hell on earth at work)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Feature non-breaking change which adds functionality v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants