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

VideoPlayer: drop streamInfo #9238

Merged
merged 1 commit into from
Mar 2, 2016
Merged

Conversation

FernetMenta
Copy link
Contributor

fighting spaghetti code
too much dependencies just for displaying redundant info on a debugging screen. all this info is available to the GUI and most of the info is displayed by the OSD anyway.

@FernetMenta FernetMenta added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v17 Krypton Component: Video labels Feb 29, 2016
@FernetMenta
Copy link
Contributor Author

jenkins build this please

@popcornmix
Copy link
Member

I do use this overlay pretty much every day for debugging purposes.
The current bitrate, confirming if we are software/hardware decoding the drops/skips counts are all vital when trying to work out why a file isn't playing smoothly. This info is not available in the OSD.

@MrMC
Copy link

MrMC commented Feb 29, 2016

same here, absolutely required to debug decoder/buffer issues that occur time and time again..

@FernetMenta
Copy link
Contributor Author

feel free to implement it again, labels are still there

@fritsch
Copy link
Member

fritsch commented Feb 29, 2016

The code just being removed here seems clean and non intrusive. What do you have in mind for a "new implementation"?

@FernetMenta
Copy link
Contributor Author

the code I have removed was maximum nonsense. demux info collected from demuxer was polled from a window through application and application player object. This is what I call crap and no-go. The info queried by GetStreamInfo was redundant. For example bitrate is a member of DemuxStream and can be accessed without the removed method.
If someone wants to display bitrate on the codec screen, this can be done like skips or drops that don't go the route over application.

EDIT:

This is what I call crap and no-go

Please don't get this wrong. I don't blame anyone. I know the code has grown over time. My judgement is only related to the current snapshot of the code, not to anybody who has written it.

@fritsch
Copy link
Member

fritsch commented Feb 29, 2016

Thanks, I mainly talked about the skips / drops display, which is really useful information to see if render was late or if decoder needs / has dropped.

It's obviously clear that there is a reason you remove it - no offense here.

@FernetMenta
Copy link
Contributor Author

Thanks, I mainly talked about the skips / drops display

That section is still there.
I agree that bitrate is useful because it can change but I doubt that a half decent developer would not know the codec or number auf audio channels of a test sample.

@fritsch
Copy link
Member

fritsch commented Feb 29, 2016

@FernetMenta I can link you a sample with 8 audio tracks (LPCM, TrueHD, AAC, FLAC, ...) all in one - used for testing ...

@popcornmix
Copy link
Member

I tested this PR and skips/drops was not visible for me.

@FernetMenta
Copy link
Contributor Author

I will bring the important parameters back with utilizing the new ProcessInfo object. But first I want to finish InputStream addon that requires this change here.

@fritsch
Copy link
Member

fritsch commented Feb 29, 2016

Thanks much!

2016-02-29 17:22 GMT+01:00 Rainer Hochecker notifications@github.com:

I will bring the important parameters back with utilizing the new
ProcessInfo object. But first I want to finish InputStream addon that
requires this change here.


Reply to this email directly or view it on GitHub
#9238 (comment).

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@FernetMenta
Copy link
Contributor Author

@popcornmix I think about rendering the debug info as overlays by renderer and drop the controls of the skin. Can you think about any reason why this should not work?

@popcornmix
Copy link
Member

@FernetMenta are you suggesting the codec info is part of the current GL overlay, or a separate layer?

@FernetMenta
Copy link
Contributor Author

@popcornmix I would render the debug info the same way as a text overlay. With regard on performance this should be better then rendering it by a skin.

@popcornmix
Copy link
Member

We don't have specific text overlays. You can create a bitmap and add it as an overlay, but kodi will need to handle writing fonts to the bitmap. Whether it will help performance or not is debatable.
You'll pay the extra overlay being fetched from sdram every refresh (e.g at 60Hz) whereas the current scheme might only be updating at ~10fps. Current scheme potentially is forcing other OSD info to be redrawn when codec info changes.

Drifting off topic a bit, it might be nice for the codec info, the debug info and the mouse pointer to rendered through an overridable method, which when overridden avoids setting the dirty flag if just that overlay has changed. I doubt all platforms can usefully support this, so I imagine a fallback to using GL/GLES would still be useful.

@FernetMenta
Copy link
Contributor Author

We don't have specific text overlays

Certainly we do. Some subtitles are text only and those type can be used for this case.

Whether it will help performance or not is debatable

I think it will. The GUI handles this as generic text, that's all information it has. If I do this from player, I know that updating values every 20-40 makes no sense because this just results in some flickering numbers.

You'll pay the extra overlay being fetched from sdram every refresh (e.g at 60Hz) whereas the current scheme might only be updating at ~10fps

No, see above. Absolutely no reason to update every refresh. Maybe I update it every second.

@popcornmix
Copy link
Member

Certainly we do. Some subtitles are text only and those type can be used for this case.

I was assuming you were talking about using an additional hardware overlay for the text, in which case the Pi, for example doesn't have support for a textual overlay (only general purpose bitrmaps).

Yes, agree that simple subtitles are doing something very similar.
But are you talking about rendering the codec info overlay to the current GL framebuffer, or using a separate hardware overlay distinct from that?

@FernetMenta
Copy link
Contributor Author

But are you talking about rendering the codec info overlay to the current GL framebuffer, or using a separate hardware overlay distinct from that?

To the same framebuffer. Most platforms only have a single one. Why do you ask?

@ghost
Copy link

ghost commented Mar 1, 2016

I came across GetStreamInfo a while ago when playing with smooth stream changes.
I was wondering, why this codec information are displayed in the overlay as it changes only if stream changes (and not on every frame).

I found a reasonable place where to look for codec Information (and where they are already displayed today: In the video / audio settings menu when playing an video.
Its not as complete as in the overlay, but gives me already useful information.
I'm sure that major missing things can be added there.

For the very deep analyse I go simply into kodis log file.

@popcornmix Are drops / skips available in the meantime on your system? I never lost them...

@FernetMenta
Copy link
Contributor Author

There's a lot of parallel work going on related to this part of the application and I don't want to deadlock here. If no objections, I would merge this and do the work on the debug info later.

@popcornmix
Copy link
Member

As long as codec info will come back, then I won't object to this now.

@ace20022
Copy link
Member

ace20022 commented Mar 2, 2016

I have no objections.

FernetMenta added a commit that referenced this pull request Mar 2, 2016
@FernetMenta FernetMenta merged commit 68263d5 into xbmc:master Mar 2, 2016
@FernetMenta FernetMenta deleted the demuxstreams branch March 2, 2016 08:33
@razzeee razzeee added this to the Krypton 17.0-alpha1 milestone Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants