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

Jf video stats #781

Merged
merged 15 commits into from
Sep 23, 2022
Merged

Jf video stats #781

merged 15 commits into from
Sep 23, 2022

Conversation

jimdogx
Copy link
Contributor

@jimdogx jimdogx commented Sep 6, 2022

Currently you can hit "down" for subtitles during playback. This PR adds "streaming information" if the user presses "up" during playback:

Screen Shot 2022-09-05 at 10 39 21 PM

Changes
Listens for "up" press during playback and shows any streaming / transcoding information available.

Issues
This is currently using the "built in" options dialog even though these are not options. The presentation does have a side benefit of highlighting individual pieces of information as the users scrolls through the list. A custom view can be created at a later dated if deemed necessary.

source/VideoPlayer.brs Outdated Show resolved Hide resolved
@alanazar
Copy link
Contributor

alanazar commented Sep 6, 2022

I've tested it and it works. Great idea for stats!

source/VideoPlayer.brs Outdated Show resolved Hide resolved
@alanazar
Copy link
Contributor

alanazar commented Sep 6, 2022

The number values after "Total Bitrate: " and "Audio Codec: " have extra spaces
Answer from 1hitsong "Roku does odd auto-aligning when dealing with numbers. Likely needs to have a left align put on the value fields."
I don't know how to do that :D

jimdogx and others added 5 commits September 6, 2022 15:20
Co-authored-by: alanazar <93149610+alanazar@users.noreply.github.com>
Co-authored-by: alanazar <93149610+alanazar@users.noreply.github.com>
Co-authored-by: alanazar <93149610+alanazar@users.noreply.github.com>
Co-authored-by: alanazar <93149610+alanazar@users.noreply.github.com>
Copy link
Contributor

@alanazar alanazar left a comment

Choose a reason for hiding this comment

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

Tested it again and looks good
Worth revisiting later to find a way to not have Mbps or Kbps as an integer (I'll happily research this later)
Also, the left alignment of number values if that's possible.
It's a great feature to add, thanks for this!

@neilsb
Copy link
Member

neilsb commented Sep 7, 2022

Looks good - only one issue I found (purely by chance) is that if you have the Video Stats dialog open, and the Skip Intro button appears then the Stats Dialog loses focus, and you can't get it back.

The video stats is now stuck on the screen until you exit the app with the home button (can't even exit app by pressing back).

@alanazar
Copy link
Contributor

alanazar commented Sep 7, 2022

Looks good - only one issue I found (purely by chance) is that if you have the Video Stats dialog open, and the Skip Intro button appears then the Stats Dialog loses focus, and you can't get it back.

The video stats is now stuck on the screen until you exit the app with the home button (can't even exit app by pressing back).

Oh good catch! I'll keep it in mind for future testing as well

@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 7, 2022

Looks good - only one issue I found (purely by chance) is that if you have the Video Stats dialog open, and the Skip Intro button appears then the Stats Dialog loses focus, and you can't get it back.

The video stats is now stuck on the screen until you exit the app with the home button (can't even exit app by pressing back).

Nice catch. I don't have that plugin installed, but will try to install it soon and re-test 👍

@1hitsong
Copy link
Member

1hitsong commented Sep 7, 2022

To piggyback off Neil, if the Skip Into button is already shown when you press up, everything works nicely until the Skip Intro button fades out. That fade out sets the focus back to the video, not the popup. Then it's stuck on screen.

@1hitsong
Copy link
Member

FYI, Intro Skipper plugin support has been removed. With that out of the way, were there any other lingering issues?

@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 20, 2022

I don't think so, but I'd wait until @neilsb chimes in to be sure 👍

@neilsb
Copy link
Member

neilsb commented Sep 20, 2022

Nope, was all good as I recall. Just would want to test it with the rolled back PR but if someone has given it the one over since then then that's cool. I can have a look at it later on today.

@alanazar
Copy link
Contributor

Last update broke the functionality.
On some videos i get a window.
On others nothing happens but then no other button works (back, play, forward, etc). I need to exit to Roku's home to then be able to access the server.

Is the bitrate what the video is reporting and not the live network traffic? If so, I suggest to revert the changes since it was working before and the missing stats were just codec dependent

@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 21, 2022

BitRate turned out to be an Integer, even though I could have sworn I looked at the server code and it said it was a String. I've updated and pushed a fix (and was able to actually test it this time instead of pushing blindly) 😄

@jimdogx jimdogx marked this pull request as draft September 21, 2022 11:41
@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 21, 2022

Converting PR to a draft so I can add more data to be shown....

@jimdogx jimdogx marked this pull request as ready for review September 21, 2022 12:07
@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 21, 2022

I've added more info, I think this should be good enough for the first release of this feature:

Screen Shot 2022-09-21 at 6 06 03 AM

If the user scrolls:
Screen Shot 2022-09-21 at 6 06 11 AM

Copy link
Contributor

@alanazar alanazar left a comment

Choose a reason for hiding this comment

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

Tested and it works great :)
(Again, really like having this feature, thanks!)

Copy link
Member

@neilsb neilsb left a comment

Choose a reason for hiding this comment

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

Looks good, and works well.

Not sure if this is an issue or how it should work, but if you have the stream details up when the video ends, the next action (e.g. return to the details screen, autoplay next episode, etc) will not happen until the popup is closed. I am not sure what the expected behaviour would be for a normal user.

@jimdogx
Copy link
Contributor Author

jimdogx commented Sep 22, 2022

Looks good, and works well.

Not sure if this is an issue or how it should work, but if you have the stream details up when the video ends, the next action (e.g. return to the details screen, autoplay next episode, etc) will not happen until the popup is closed. I am not sure what the expected behaviour would be for a normal user.

I think this is acceptable. You can't move on until you close the popup. Otherwise, we'd have to implement some special logic for the popup to "refresh" with the next video playing etc.

source/VideoPlayer.brs Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
source/VideoPlayer.brs Outdated Show resolved Hide resolved
jimdogx and others added 4 commits September 22, 2022 11:24
Keep forgetting we have an isValid function.

Co-authored-by: 1hitsong <3330318+1hitsong@users.noreply.github.com>
@1hitsong
Copy link
Member

To be merged 9/23 unless an issue arises.

@1hitsong 1hitsong merged commit 1832dfd into jellyfin:unstable Sep 23, 2022
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.

4 participants