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

[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music #17976

Merged
merged 1 commit into from Jun 14, 2020

Conversation

jjd-uk
Copy link
Member

@jjd-uk jjd-uk commented May 31, 2020

Set of improvement to video fullscreen info, music visualisation and seekbar which are designed to produce a consistent styling across Movies & TV Shows, PVR, and Music.
Out of the 3 different styles currently used, the fullscreen info style for PVR was chosen as the template on which to base the changes for video fullscreen and music visualisation.

1 Music Visualisation

The looks for Music has changed the most as it wasn't possible previously to have full fidelity fanart or visualisation as:
a. A colordiffuse="88FFFFFF was applied to fanart, most likely to aid with contrast for the label text. It also allows visualisations to be seen through the fanart (not sure if this is an intentional feature).
b. An overlay from <include>ColoredBackgroundImages</include> was always applied.

Changes:
1.1. In order to remove the colordiffuse over fanart an overlay was introduced to provide the necessary contrast for text, the texture used is dialogs/dialog-bg-nobo.png which is the same texture for video fullscren info. This is only for when visualisation is not enabled, for the case where a visualisation is enabled colordiffuse was kept to retain the ability to see visualisation through the fanart.
1.2. To make it consistent with video fullscren info, a layout of Artwork to the left (cover) and the info to the right of the artwork was chosen.
1.3 More prominence given to Track No. & Title by making it bold and a larger font that the other details.
1.4 In Top Bar Overlay adjust location of position of track in playlist count, and add next track details.
1.5. Genre has been added to the info
1.6. Star rating have been moved along side the other info.

2 Movies/TV Video Fullscreen

2.1. Style changes made to match the existing style for PVR which uses with a fullwidth overlay on with the info is placed.
2.2. Chapter info was located on the seekbar and only showed the chapter numbers. I wanted to introduce Chapter Names however I felt there would not be enough room for long chapter names in this location, so it seemed to me that a good location fit would be in the Top Bar Overlay below the Movie/TV Show title.
2.3. For TV Shows show the Next info for the next item in the video playlist if it is a TV episode. This is added to below the existing Top Bar Overlay info. Wasn't sure if a more general next video playlist item info would be wanted, so it's restricted to TV shows only for now.

3 PVR Video Fullscreen
As this was the template used for everything else, only very minor dimension changes done to ensure consistent spacing's/margins of 20pt.

4 Common changes to seekbar

4.1. Removing Chapter info from seekbar allowed moving the media flags to the left. For video the media flags are now present in 2 row, one for video flags and one for audio flags. Part of the reason for this separation is the debate on #11693 on whether to use media flags to show video & audio bitrates. This gives more room for additional flags to be added.
4.2. With media flags moved to the left, the Time Remaining info can now be displayed when fullscreen info is active, previously it would only show when seekbar was active. It is now also shown for music.
4.4. Adjustments made to $VAR[SeekTimeLabelVar] and $VAR[SeekLabel] so the animation to move $VAR[SeekTimeLabelVar] up to show seek status is no longer required

Screenshots

Music Info Before
image

Music Info After
image

Music Fullscreen Fanart Before
UdJZrP2

Music Fullscreen Fanart After
3D1BKyH

Movies Before
dPEcIWL

Movies After
image

TV Show Before
h2l1KJl

TV Show After
image

PVR Before
hS5Vyfy

PVR After
image

@jjd-uk jjd-uk requested a review from ronie May 31, 2020 13:57
@jjd-uk jjd-uk added Component: Skin Type: Improvement non-breaking change which improves existing functionality v19 Matrix labels May 31, 2020
@jjd-uk
Copy link
Member Author

jjd-uk commented May 31, 2020

@ksooo @phunkyfish can one of you check PVR please, it should be pretty much unchanged apart from some minor adjustments to standardise on 20pt margin/spacings, but can only test with PVR demo.

@sarbes this will enable you to have the visualisation you developed without any overlays.

@ksooo
Copy link
Member

ksooo commented May 31, 2020

Why have you chosen to put the stream info labels in two rows? Personally, I like one row more.

And I'm not sure that moving these labels from right to left side does not have any bad side effects. IIRC, for PVR a "currently recording" label is placed on left if you pull up the info OSD for a channel currently recording.

@jjd-uk
Copy link
Member Author

jjd-uk commented May 31, 2020

Looks like I indeed missed "currently recording" label over on that side, the other potential conflicts have already been moved. Will need to think of best way forward.

@jjd-uk jjd-uk added the WIP PR that is still being worked on label May 31, 2020
ronie
ronie previously requested changes Jun 1, 2020
Copy link
Member

@ronie ronie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

big thumbs up @jjd-uk for revisiting those dialogs and trying to unify them as well.

i've finished the code review and will start with some runtime testing asap.
it would be great if our other team skinners could take it for a spin as well.
pinging @HitcherUK @gade01 @sualfred @jurialmunkey @jeroenpardon

addons/skin.estuary/xml/Custom_1109_TopBarOverlay.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Custom_1109_TopBarOverlay.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Custom_1109_TopBarOverlay.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Custom_1109_TopBarOverlay.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Custom_1109_TopBarOverlay.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/MusicVisualisation.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/MusicVisualisation.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/MusicVisualisation.xml Outdated Show resolved Hide resolved
addons/skin.estuary/xml/Variables.xml Outdated Show resolved Hide resolved
@ronie
Copy link
Member

ronie commented Jun 1, 2020

and now for my personal preferences (and you should take them that way) :-)

  1. the mute icon has a consistent place all throughout the skin.
    should that be sacrificed for a label thats only visible in the seekbar?

  2. i'm not a fan of the double rows for mediaflags, it just doesn't look right to me.
    i'd prefer to stick with 5 (or 6 if enough space) flags on a single row.

  3. i've reviewed your screenshot above and suffered from a brain crash when trying to process the info shown on the last one...
    displaying 11 different time labels in a single window caused an information overload on my end.
    error

@walterheisenberg
Copy link

sorry for hijacking
If you are re-thinking PVR, then PVR-OSD should also be rethought.
Here is a screenshot, which shows a IMHO better OSD for PVR:
https://forum.kodi.tv/showthread.php?tid=335735&pid=2932070#pid2932070

PVR is much different from Music or Videos because it has a following program and you should easily see, whats next on this channel and when it begins.
Even this OSD need tweaking e.g. double "time", no weather but this would IMHO be a improvement for all PVR users out there.
If you don't want such a discussion here, please let me know where ;)
CU

@ksooo
Copy link
Member

ksooo commented Jun 2, 2020

Please keep that discussion on the forum.

@jjd-uk jjd-uk force-pushed the Estuary_Fullscreen branch 2 times, most recently from d897129 to 055fb5d Compare June 4, 2020 16:42
@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 4, 2020

@ronie @ksooo Updated and screenshots have been update to show current state.

  • Mute icon - Changed back to how it was. It's only on my dev PC I noticed the slide animation to the left for End Time to make room for the Mute icon so it's not a big deal to me. Besides one of the points of this PR is to introduce more consistency so yes it's probably best to manintain this both in & out of fullscreen playback.

  • Media Flags - Changed media flags back to single row. The dual rows to split video & audio was something I wanted to try out because of the discussions on [estuary] added: Audio/video bitrate info for video process dialog #11693 so this would give more room for additional flags.

  • Currently recording/PVR - For PVR I've now moved the "currently recording" status to the top. To me this makes more sense as every commercial PVR I've used puts the recording status alongside the name of the TV programme being recorded. Other than this and the minor position adjustments, PVR is completely unchanged.

@ksooo
Copy link
Member

ksooo commented Jun 4, 2020

@jjd-uk I have seen single line stream info badges in the lower right at other places in Estuary. Your change to move these badges to lower left for some views would make this inconsistent, I'm afraid. Example:

screenshot000

@jurialmunkey
Copy link

On the topic of the codec badges and also relevant to Ronie's comment about time labels:

I think it is overkill to show both "time remaining" and "ends at". To keep skin positioning consistent, I'd drop the time remaining label and move the codecs back to the bottom right. That would leave the bottom left blank, but I think it is important to remember that we don't need to fill every bit of screen realestate - sometimes a bit of breathing room is a good thing!

As a separate point - is there any reason why in PVR the "Next" item is shown in the info panel with the plot but in movies/episodes/music it is shown in the top left? Shouldn't the next item be shown in a consistent position?

I personally would put the "Next" item with the plot (like in the PVR screenshots). That way the top left only relates to what is directly only screen "Now", then the plot box provides information about what is coming "Next" (plot is essentially about the future as it's purpose generally is to describe what will happen in the episode)

@howie-f
Copy link
Contributor

howie-f commented Jun 5, 2020

To keep skin positioning consistent, I'd drop the time remaining label and move the codecs back to the bottom right.

true 👍 just wanna throw in that IMO removing "time remaining" is not needed then as it never shows up with codec badges at the same time

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 5, 2020

While I agree with some of the above comments, I was trying to improve what we currently have without doing a radical redesign.

To address all the points would IMHO require a complete redesign, something like a partially done experiment of mine shown below. I've not continued it as I thought it would be too big a change from what we currently have, so these are screenshots from an unfinished work.

image

image

@ksooo
Copy link
Member

ksooo commented Jun 5, 2020

I still like best where the stream info badges are located today - lower right.

@ksooo
Copy link
Member

ksooo commented Jun 5, 2020

Forgive me, but the last PVR screenshot (your experiment) looks quite crowded to me. So much information on such a small space...

@jurialmunkey
Copy link

Forgive me, but the last PVR screenshot (your experiment) looks quite crowded to me. So much information on such a small space...

Yeah I wasn't suggesting anything as drastic as that.

Here's a screenshot of what I was suggesting:

Honeyview_screenshot004

@ksooo
Copy link
Member

ksooo commented Jun 6, 2020

Here's a screenshot of what I was suggesting:

For PVR, channel number and channel is important information. This must not be removed from the info screen, imo.

@jurialmunkey
Copy link

jurialmunkey commented Jun 6, 2020

Here's a screenshot of what I was suggesting:

For PVR, channel number and channel is important information. This must not be removed from the info screen, imo.

It's not removed. That screenshot is for tvshows/movies not PVR.

PVR would stay the same as jjd-uk's original, just with codecs moving back to the right.

The only changes to this PR that I'm suggesting are moving codecs back to bottom right and moving next playlist item into the plotbox to make it consistent with PVR.

This is PVR with that change (this is jjd-uk's actual code but with codecs moved back to right)

Honeyview_screenshot005

@jjd-uk jjd-uk force-pushed the Estuary_Fullscreen branch 2 times, most recently from 7560578 to bbb838c Compare June 6, 2020 15:29
@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 6, 2020

Updated again, hopefully the final design iteration this time. Screenshots have been update to reflect current state.

I noticed on jurialmunkey's he had a show with a long name, so decided to split episode and chapter info onto separate lines.

@phunkyfish
Copy link
Contributor

So PVR is unchanged? I don’t see a difference in the screenshots now.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 6, 2020

So PVR is unchanged? I don’t see a difference in the screenshots now.

@phunkyfish There was never much change for PVR as I used PVR style as template for all the other changes.

With latest version the difference from master is even less now, what's left is:

  • Standardise a 20pt margin between elements on x axis,
  • Next label uses button_focus color to fit with other uses of next elsewhere.
  • Text color of currently recording changed from white to red,

So only thing from PVR point of view that needs double checking, are the the co-ordinate changes to provide for the 20pt margins as I can only test with PVR Demo.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 6, 2020

@phunkyfish if you're able to take a look, this bit I had to adjust blind

<control type="textbox">
<left>240</left>
<top>10</top>
<right>240</right>
<height>160</height>
<label fallback="19055">$INFO[VideoPlayer.Plot]</label>
<align>justify</align>
<autoscroll delay="5000" repeat="7500" time="5000"></autoscroll>
<visible>!String.IsEmpty(PVR.EpgEventIcon)</visible>
</control>
<control type="image">
<right>20</right>
<top>20</top>
<width>200</width>
<height>200</height>
<aspectratio aligny="center">keep</aspectratio>
<texture fallback="DefaultTVShows.png">$INFO[PVR.EpgEventIcon]</texture>
<visible>!String.IsEmpty(PVR.EpgEventIcon)</visible>
</control>
as with PVR Demo I don't see anyway to get that PVR.EpgEventIcon. It's only minor adjustments for the left & right co-ordinates.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 12, 2020

@ronie cosmetics fixed

@ronie ronie merged commit f9ef941 into xbmc:master Jun 14, 2020
@ronie
Copy link
Member

ronie commented Jun 14, 2020

thx!

@ksooo
Copy link
Member

ksooo commented Jun 14, 2020

I noticed two things with this (otherwise nice) rework with TV shows info:

screenshot001

@ksooo
Copy link
Member

ksooo commented Jun 14, 2020

Regarding the problem with "Next:" I mentioned in my previous comment - forget about it. I was using an inconsistent build. Sorry for the noise.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 14, 2020

@ksooo that timestamp for the chapter is returned by core if there's no chapter name, it is exactly the same as it was in Confluence. I don't believe there's any way for me to reduce precision at a skin level.

@ksooo
Copy link
Member

ksooo commented Jun 14, 2020

which infolabel is that?

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 14, 2020

@ksooo It's $INFO[Player.ChapterName]

@ksooo
Copy link
Member

ksooo commented Jun 15, 2020

@jjd-uk fyi, in my case the chapter title (player.chaptername) comes from ffmpeg - so nothing we can easily fix, because ffmpeg returns this as chapter title.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 15, 2020

@ksooo I guess there's some logic somewhere that if there's no chapter name it falls back to the timestamp used for seeking to chapters? so no surprise that timestamp comes from ffmpeg as it's a player infobool, or are you saying it's ffmpeg always used so the fallback is in ffmpeg? Personally I wouldn't mind if we only returned a value if it's string so the timestamps never get displayed, no idea whether that might be possible.

@ksooo
Copy link
Member

ksooo commented Jun 15, 2020

guess there's some logic somewhere that if there's no chapter name it falls back to the timestamp used for seeking to chapters?

Yes, and unfortunately this logic seems to be inside ffmpeg, not Kodi code. :-/

or are you saying it's ffmpeg always used so the fallback is in ffmpeg

The demuxer is asked for the title, so the value is what the demuxer returns. No logic inside Kodi. This can be different for ffmpeg or inputstream.adaptive, ... ffmpeg demuxer asks ffmepeg for the title, another demuxer will do something else. From Kodi perspective it is just an arbitrary string returned by the demuxer if asked for the chapter name.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jun 15, 2020

Got it.

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 16, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 21, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
@jjd-uk jjd-uk mentioned this pull request Jul 7, 2020
11 tasks
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[Estuary] Improvements to Fullscreen Info/Seekbar for Video & Music
@jjd-uk jjd-uk deleted the Estuary_Fullscreen branch August 8, 2020 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Skin Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants