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

fix false 4K detection on some 1080i sources #3680

Merged
merged 1 commit into from Jan 23, 2014

Conversation

Jalle19
Copy link
Member

@Jalle19 Jalle19 commented Nov 18, 2013

No description provided.

@fritsch
Copy link
Member

fritsch commented Nov 19, 2013

That one is fine. Thanks much for looking into it.

@FernetMenta
Copy link
Contributor

hmm, this is not really correct. if 1088 is provided as height, the source of information is wrong (most likely tvh) and does not consider cropping.

@Jalle19
Copy link
Member Author

Jalle19 commented Nov 19, 2013

@FernetMenta I didn't dig that deep into the code to see where the 1088 value comes from, could you give me any pointers?

@fritsch
Copy link
Member

fritsch commented Nov 19, 2013

4K is somewhere far away. Though the content might not really match "dimension width" cause the Tool that has written it made mistakes, I still think that even: height: 1714 && width: 3656 (according to: http://en.wikipedia.org/wiki/4K_resolution ) is a good value in general to indicate 4K

@opdenkamp
Copy link
Member

1088 is indeed provided by tvheadend, which indeed doesn't consider cropping. it just passes whatever is provided by the mux descriptors.

@FernetMenta
Copy link
Contributor

the problem here is that some decoders could refuse to open when height is 1088 instead of 1080. tvh parser just need to parse a few bits more of sps and subtract cropping parameters from height/width.

@fritsch i agree that maybe 4k should be further specified but here we are in the 1080 block. means the 1088 would fall under a new category undetermined.

@opdenkamp
Copy link
Member

@adamsutton ^^

@adamsutton
Copy link
Contributor

@opdenkamp not really my area, you know me and codecs. I don't know what the full implications would be, I'll discuss with @andoma et al, and see what the say.

Thanks for the heads up though.

@opdenkamp
Copy link
Member

might need 2 new fields in htsp if @andoma relies on the 1088 width (which I doubt)

@FernetMenta
Copy link
Contributor

@adamsutton
Copy link
Contributor

@FernetMenta @opdenkamp just discussing with @andoma right now, he agrees subtracting cropping in TVH makes sense. He'll probably add a patch.

@andoma
Copy link

andoma commented Nov 19, 2013

How about this: tvheadend/tvheadend@d0af361

@Jalle19
Copy link
Member Author

Jalle19 commented Nov 19, 2013

So is tvheadend the only addon that doesn't account for the extra 8 pixels?

@opdenkamp
Copy link
Member

thanks @andoma, i'll have a look at it when i got time this evening.

@Jalle19 yes, only vdr and tvh provide their own demux

@adamsutton
Copy link
Contributor

@Jalle19 I'd assume that TVH and VDR are the only two affected at all

@Jalle19
Copy link
Member Author

Jalle19 commented Dec 2, 2013

The fix has made it into master: tvheadend/tvheadend@1b78435

I still think we should keep this change (regardless of the underlying cause) since 1920x1088 is not 4K under any circumstances.

@elupus
Copy link
Contributor

elupus commented Dec 15, 2013

Ffmpeg demuxer reported without cropping for a long time too.

@Jalle19
Copy link
Member Author

Jalle19 commented Jan 7, 2014

Any news on this? We need some kind of fix for Gotham. Someone else has noticed it too: http://forum.xbmc.org/showthread.php?tid=182488

@FernetMenta
Copy link
Contributor

i would make it obvious (comment and commit message) that the root cause is tvh which sends incorrect height and this is only a work-around.

@adamsutton
Copy link
Contributor

@FernetMenta Though I acknowledge that TVH wasn't providing accurate information for what it was trying to say and that has been fixed. I would also think it sensible to make it clear in the comment that the original test is rather invalid given that 4K = approx 4K vertical pixels and 1088 is in no way anywhere near 4k pixels.

@FernetMenta
Copy link
Contributor

I would also think it sensible to make it clear in the comment that the original test is rather invalid given that 4K = approx 4K

absolutely agree. as fritsch pointed out 4k starts way beyond 1088. maybe we should put something between 1088 and 4k.

@Jalle19
Copy link
Member Author

Jalle19 commented Jan 7, 2014

Github lost my e-mail reply, here it is:

Seeing as 3840x1600 (4K at 2.40:1 aspect ratio) is 6,144 megapixels and 2880x2160 (4K at 4:3) is 6,219 megapixels, maybe we could classify anything with a pixel count over 6M as 4K?

@fritsch
Copy link
Member

fritsch commented Jan 7, 2014

Update your PR and we will see.

@da-anda
Copy link
Member

da-anda commented Jan 7, 2014

please take stereoscopics into account. We also don't want to show 4k labels for FullSBS 3840x1080 or FullOU 1920x2160. But we should be save on this if you use the 6M detection - nontheless we could/should at some point take 3D detection into acount (not sure if the item property holding the stereomode info is available at that point)

@Jalle19
Copy link
Member Author

Jalle19 commented Jan 8, 2014

@da-anda that shouldn't be a problem if we put the limit at 6M (your examples are both a bit over 4M). I'll update the PR later today.

Older versions of
tvheadend incorrectly report 1080i channels as having 1088 vertical
pixels so they got classified as 4K. The algorithm is now changed
to consider source as 4K only when the total pixel count exceeds
6 megapixels.
@Jalle19
Copy link
Member Author

Jalle19 commented Jan 13, 2014

Updated and rebased.

@da-anda
Copy link
Member

da-anda commented Jan 18, 2014

http://trac.xbmc.org/ticket/14835 is also affected by wrong 4k detection and should be resolved once this one is merged

@t-nelson
Copy link
Contributor

@ronie @jmarshallnz how will skins handle the new empty return for unknown
dimensions? That's my only real concern.

@jmarshallnz
Copy link
Contributor

I'm guessing they already handle it as files without size info won't show anything?

@da-anda
Copy link
Member

da-anda commented Jan 22, 2014

jenkins build this please and @t-nelson merge this please? :)

@MartijnKaijser
Copy link
Member

skin won't show anything if value is empty.
they use "ListItem.VideoResolution.png" so if value is empty it tries to display ".png" instead of "4k.png"

@ronie
Copy link
Member

ronie commented Jan 22, 2014

either that, or they will display a predefined fallback image ;-)

@t-nelson
Copy link
Contributor

Ok when Jenkins says so.

da-anda pushed a commit that referenced this pull request Jan 23, 2014
fix false 4K detection on some 1080i and 3D sources
@da-anda da-anda merged commit 1ae61a6 into xbmc:master Jan 23, 2014
@Jalle19 Jalle19 deleted the fix-4k-detection branch March 5, 2014 09:19
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.

None yet