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

Player.Process info label changes #10069

Merged
merged 6 commits into from
Jul 5, 2016
Merged

Conversation

FernetMenta
Copy link
Contributor

No description provided.

@FernetMenta FernetMenta added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality labels Jul 3, 2016
@sualfred
Copy link
Member

sualfred commented Jul 3, 2016

I've tested Hitcher's triggered test build:

What works:

  • Video FPS
  • Video AR
  • Pixel format
  • Video decoder

Doesn't work:

  • Deinterlace method always unkown
  • Audio channels always unkown
  • Audio decoder always unkown
  • Video height/width/sample rate/bits per sample are completely empty without "unkown"
  • HW decoder bool is false even if DXVA is used
  • Info labels won't be cleared if playback is stopped

@FernetMenta
Copy link
Contributor Author

Video height/width/sample rate/bits per sample are completely empty without "unkown"

You know that those are integer?

HW decoder bool is false even if DXVA is used

not set by DXVA. platform devs need to set this info in their codecs

@FernetMenta
Copy link
Contributor Author

I see the issue why audio related info is empty. Will fix soon.

@stefansaraev
Copy link
Contributor

You know that those are integer?

sadly (or not), skins seems not to care.

not set by DXVA. platform devs need to set this info in their codecs

once, kodi was multi platform application, and feature PRs were supposed to be "complete", especialy when they were supposed to re-add a functionality dropped by intention.

did you runtime test this, @FernetMenta ?

@sualfred
Copy link
Member

sualfred commented Jul 3, 2016

Ah okay.
Since the values are static, doesn't it make more sense to output them as string? These Integers cannot be used to fill a label. Or do you had a different purpose for it?

@FernetMenta
Copy link
Contributor Author

@sualfred bear with me, my skinning skills are limited. If you want strings, you get strings :)

@FernetMenta
Copy link
Contributor Author

@stefansaraev what you are saying makes no sense. I progress this in iterations. No need to have AML implemented for this testing here. Why should we block skinning work waiting for all platforms to catch up.

@stefansaraev
Copy link
Contributor

did I say something about aml. @FernetMenta ?

@sualfred
Copy link
Member

sualfred commented Jul 3, 2016

@FernetMenta
That's why I was asking, my coding skills are very limited ;)
I think strings would be better in this case.

@HitcherUK @phil65 Are you fine with strings?

@stefansaraev
Copy link
Contributor

@FernetMenta

it makes perfect sense, and it is obvious that you never runtime tested this.

first you broke it, without even knowing the potential colateral damages you are doing, calling others code (that worked for years) "crap" (several times), then you shoot down everyone that makes valid points (yea, pvr signal info is completely unrelated here, of course)

we have been repeating this "catch up" thing for 2 releases now, enough is enough.

-1 to this PR til its fully working for all codecs we have in tree, including skin support, and everything we had in the old codecinfo screen works as before (or better).

@FernetMenta
Copy link
Contributor Author

@stefansaraev your -1 does not count here. I won't implement for all codecs and we don't have an Android dev. Following your advice is silly because we would never get this new functions.

@FernetMenta FernetMenta added this to the Krypton 17.0-alpha3 milestone Jul 3, 2016
@stefansaraev
Copy link
Contributor

of course it does not count (I did not expect a benevolent dictator for life would count it)

@stefansaraev
Copy link
Contributor

feel free to completely drop the android port, btw. I am leaving now.

@FernetMenta
Copy link
Contributor Author

I only ignore your comments on this thread. This is a dev space, you should know. I don't consider you as a skilled developer in this domain, hence I don't consider your vote.

@MartijnKaijser
Copy link
Member

@FernetMenta I warn you and show some respect towards others! You are going to far again.

@FernetMenta
Copy link
Contributor Author

direct this warning to seo as well. you were around a couple of days ago when he was bitching at me on slack. it is hard giving respect to someone hahaving like this.

@FernetMenta
Copy link
Contributor Author

@sualfred I change width, height, samplerate, bitpersample to string
isHw should work now too. audio related info gets updated.

Do you want me to kick off a test build for Windows?

@sualfred
Copy link
Member

sualfred commented Jul 4, 2016

@FernetMenta
Yep, please trigger one.

@sualfred
Copy link
Member

sualfred commented Jul 4, 2016

@FernetMenta
Tested. Everything works well except the deinterlace method. This value stays at "unkown".

Couldn't test PVR and DASH sources, because I'm in the office.

@Hitcher
Copy link
Contributor

Hitcher commented Jul 4, 2016

That should be 'Unknown' by the way ;)

@FernetMenta
Copy link
Contributor Author

hehe, the only "unkown" I was able to find is in SFPTFile: https://github.com/xbmc/xbmc/search?utf8=%E2%9C%93&q=unkown

@sualfred are you sure it reads "unkown"?

@amet
Copy link
Contributor

amet commented Jul 4, 2016

https://github.com/xbmc/xbmc/blob/master/addons/resource.language.en_gb/resources/strings.po#L5944

g_localizeStrings.Get(13205) disagrees :)

edit: hehe, dyslexia kicked in

@sualfred
Copy link
Member

sualfred commented Jul 4, 2016

@FernetMenta
Yep. Tested it with my media here and with a interlaced sample video. It stays at unkown even if deinterlacing is forced by default.

@FernetMenta
Copy link
Contributor Author

@sualfred yes, that is not implemented yet. The question was if it says "unknown" or "unkown" :)

@sualfred
Copy link
Member

sualfred commented Jul 4, 2016

@FernetMenta
Sry, too blind to read my own typo ;) It's "unknown".

Edit:
http://i.imgur.com/Q3epEJw.png

@Hitcher
Copy link
Contributor

Hitcher commented Jul 4, 2016

q3epejw

Saved me from coding it, thanks.

@sualfred
Copy link
Member

sualfred commented Jul 4, 2016

Always at your service :)

@FernetMenta
Copy link
Contributor Author

jenkins build this please

@FernetMenta FernetMenta merged commit 76b5a4b into xbmc:master Jul 5, 2016
@FernetMenta FernetMenta deleted the codecinfo branch July 5, 2016 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants