native resolution ( disable upscaling ) option #1096
Conversation
|
Just before even reviewing this. We can't merge stuff without proper commit messages. "Update master" is a bit to general in that case ;). You can change your commit message by doing the following on your branch: git rebase HEAD~3 -i then before each line put a "r" for reword. After that you can change the commit messages. Then you can update this PR by issueing: git push -f origin master (assuming origin is your remote - not ours ;) ). |
|
Overall looks good to me (after adressing the mentioned things). And you should consider to prepare the needed documentation in the wiki for the new advancedsetting. I'm not sure if this approach will disturb the refreshrate adaption. From a quick look it could be that it will prefer the best fitting resolution and therefore skip a fitting refreshrate couldn't it? |
|
ok, added a lot of cosmetics ;) |
no problem - let me know in what form do you need it (email or comment here, or... )
it searches for the best resolution and then it looks for refreshrate closest to the original one. if the original screen is 1920x1080@24.00Hz and say source is 624x352, then it is finding first 640x480@70.00Hz, then 640x480@60.00Hz and stays with 60Hz as there is no more good dimensions. Anyway, this automatic search is first approach to this feature. It can be heavilly parametrized, I can imagine for example that instead of finding the best match, we can define several dimension windows, or create rule that all SD contents always falls into 800x600@60Hz etc. |
|
This won't work when GUI is 60 htz and u play a 24hrtz:its going to set 60 according to above p9st |
|
Ok some more thoughts. If a user has turned on refresh rate adaption - i bet he is doing this because of micro stuttering when refreshrate is not matched to the framerate. So in that case we have to ask us the question: What is worse - having XBMCs upscaling or having microstutter. Imho microstutter is worse. In that case we should do the following:
Another question is:
And the last thing. Once we have consensus here. Since you changed your commit message for all 3 commits to the same - you can just squash all your commits down to 1 when everything is in place at the very end of this (no need to have 3 or 4 commits for this one feature). Well lets hope for other comments because this isn't really my sector ;) |
well I rather think it will have more values in near future (see my previous comment). but before this will be extended with other options I would rather wait for feedback from users, to see where is the immediate need to enhance it
yeap, makes sense, I will update the code
I do not think there is other option here - see examples in the initial post, most of SD resolutions are something like:
sure, will do |
|
ok, I merged it and changed also the setup. now the advancedsettings.xml setup looks like this: the < forcewidezoom > option requires some explanation. when < nativeupscale > is enabled, the assumption is that finally the output will be upscaled by external device to some high resolution (like 1920x1080). therefore we want XBMC to generate output filled with the video contents as much as possible (minimize black bars), as it will be stretched by the receiver anyway. the WideZoom option works perfect for this, so zooming can be forced here for SD contents, before receiver will finalize the picture. After all this looks pretty ok. I tested these changes on the Linux version of xbmc. |
|
When you have 624x352 and choose 640x480, what's the point - you're scaling it twice at that point. I don't have a problem necessarily if there is no scaling at all done by XBMC, but if XBMC is scaling as well as the TV that seems counterproductive? |
|
I think the point is here: http://www.marvell.com/digital-entertainment/qdeo/ |
|
The processing doesn't run when not resizing? This is the point of the patch after all, to have better resizing done by your TV/receiver or whatever. If XBMC is resizing (poorly apparently) then you want to eliminate that completely, else you're introducing a source of error. In the case of a 624 wide image, for example, you could center it in a 640 wide resolution to save any resizing at all by XBMC. Again, I have no problem for 1280 wide sources and the like - just not sure I see the point of resizing at 2 separate levels if the entire point was to remove the resizing to begin with. Perhaps as this problem will likely only be seen on SD sources, it doesn't really matter too much? |
|
well, not really. the xbmc is always resizing the image, so the error is here already. resizing to smaller dimension is however creating smaller image errors, and then the processing chip enhances it even more. so at the end the image looks much better than original image fully processed by xbmc. |
|
It'll only be 8 pixels either side in the 624 width case, and nothing at all if the source was 640 wide or 1280 wide etc. The latter is where the real benefit comes as XBMC does no resizing. |
|
I think the height is bigger problem than width, as usually SD contents fits very well either to 640 or 720 wide. Anyway all these examples which I tested above were looking much better with initial WideZoom scalling. Source 720x304 on resolution 720x576 gets almost 50% of horizontal black bars, and when upscaled by receiver to 1920 wide it looks like 70% of the screen is black. really ugly. well, yes, all this makes sense only for SD enhancements, the HD contents does not really need all this. the proposed patch will not affect the HD contents though. |
|
Imho the keepfps option is a dupe of the refreshrate adaption in the gui. Get rid of it and use the gui option instead. |
|
yeah, I was considering it initially. but at the end I think it makes more sense to keep it separately. The refresh rate adaption works for all your contents (HD, SD, whatever) - it is system wide parameter. When enabling < nativeresolution > adoption it is more convenient to keep all its options in the same place (override fps or not, force wide zoom or not etc). at least that was my thinking. |
|
actually the intention was to apply this check only if both values are set. I'm not sure if enabling it when only one size is set (width or height) is practical? |
|
Just tested it and stumbled over it. Up to you what this should do - i don't have an opinion on that. Well so far i compile tested this on osx, ios, linux and Windows. And runtime tested it on osx (testing the other platforms shortly). |
|
On windows i get 2 compile warnings. To fix use double instead of float for your variables (else it complains to stick a double in a float in line 105 because calculation is done in double) |
|
updated |
|
Ok - i did runtime checks for all platforms now (osx,ios,atv2,linux,windows). It does what it should on all of them including ios with tvout (though i wouldn't do it there because switching resolutions is pain slow and we are loosing audio due to hdmi handshake audio b0rk). So jm - at least from the technically POV i would sign this off for at least osx/ios (and if nobody else tests i sign it off for linux and windows too ;) ). |
ghost
assigned 
|
I wont be able too look at it this week. But fully native scaling make |
well, I do not have any good argument for upscaling, except what I saw on the screen: http://forum.xbmc.org/showthread.php?tid=64139&pid=1136328#pid1136328 |
|
ok, I corrected it according to suggestion made by jmarshallnz, now the image is streteched to aspect ratio of the final screen (most probably 16x9). The setup changed a little: I tested briefly these changes on the Linux version of xbmc. I would need to test it a little more though. |
|
ok, I'm done. It works great for me. I tested these changes on the Linux version of xbmc. |
|
sorry to enter the discussion so late. |
|
Do you mean it should disable it (VS_DEINTERLACEMODE_OFF) each time when nativeupscale is activated? Or only do this for specific resolutions or fps? |
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
From adam-aph pull xbmc#1096 Rebased to master with appropriate code change Removed AdvancedSettings and move them to a struct with sensible defaults
http://forum.xbmc.org/showthread.php?tid=64139&pid=1131505#pid1131505
I spent few hours to dig into it and the results are promising. I'm pretty fresh to xbmc though, so maybe there is better way to deal with it. Anyway, what you need to run native resolution:
How it works:
Here are few examples, where
USER = default screen resolution
NATIVE = movie resolution
ADJUST2 = final xbmc resolution for playback
I have to say that results are surprisingly good. I'm using receiver with Marvell Qdeo chip and it is doing great job, the picture is visible better than when xbmc is upscaling directly to 1080p. Well, you need to try yourself to judge.
The function used to find the closest resolution match is very simple, perhaps it might be improved. For now it tries to find the lowest resolution which is equal or higher than source resolution and tries to keep as close as possible to original refresh rate for the display.