[infomanager] only update a/v info if something has changed and only by ...

[FernetMenta]

follow-up from #5673

@herrnst could you please try again. Now it triggers the update of channels at the correct pos.

[herrnst]

With the mentioned testclip (and a mkv with a DTS track switching from 6 to 7 channels after the studio logo), the audio channel count now is updated.

Can't tell though if it'll crash or not ATM, since for me the initial issue never happened until yesterday while trying to reproduce the user reports with different XBMC/Kodi distributions (plain Linux, OpenELEC etc.). However, I could greatly amplify (crash in 1-2 secs) the issue by commenting the condition at https://github.com/xbmc/xbmc/blob/master/xbmc/GUIInfoManager.cpp#L4324 and was safely eliminated after #5669, and doesn't seem to apply with your approach.

[FernetMenta]

Now update of the cache is only done by the main thread which should cure the crash as well and eliminates the need for a lock at this position.

[herrnst]

As mentioned, for me it literally never happened, first time was yesterday when trying to investigate again, it was mostly forum users where it happened. Update-wise, I also tried a live stream, track info gets updated as usual when the stream server updates the meta info. Not seeing problems after this short period of testing.

[topfs2]

Probably needs OMX player love. And what about external players? Not sure if we query them for this though.

Somehow this feels like changing the api a bit. Which will break third party players. Which we might not care about, but needs discussion

[FernetMenta]

you are right about OMX player, there is still the audio and video part.
regarding external players, they don't have implemented GetVideoStreamInfo/GetAudioStreamInfo so nothing changes.

[fritsch]

jenkins build this please

[topfs2]

Any particular reason why we don't add this to the IPlayerCallbacks instead? Feels very hacky to have players call directly into the info manager IMO.

[FernetMenta]

a callback is not the right concept. you can see this when you look at the parameter destination rect which does not really belong to the video info struct and needs to be updated by renderer. all the data currently being polled from all over the place should finally go to some global cache where the gui and external clients can read from.

[topfs2]

I don't see why callback isn't correct? Its the way players tell the
outside world that something had changed. Same with next song etc, all that
propagates to GUI without player calling directly to GUI info manager.
Which also shouldn't be assumed exists. (Think headless)
On 10 Nov 2014 11:17, "Rainer Hochecker" notifications@github.com wrote:

a callback is not the right concept. you can see this when you look at the
parameter destination rect which does not really belong to the video info
struct and needs to be updated by renderer. all the data currently being
polled from all over the place should finally go to some global cache where
the gui and external clients can read from.

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

[FernetMenta]

do you intend to implement callbacks for renderer and all other components in the future? there is data like output rect which does not belong to player. player won't know about a change and can't inform whatever sits at the other end of the callback

and further callbacks are brittle in this case because decoder needs to inform audio player which needs to inform player. might be even more levels.

[topfs2]

The player is feeding the data to the renderer so I assumed it knew about
the data :) is there some video size the renderer decides which the player
knows nothing about?
On 10 Nov 2014 15:13, "Rainer Hochecker" notifications@github.com wrote:

do you intend to implement callbacks for renderer all other components in
the future? there is data like output rect which does not belong to player.
player won't know about a change and can't inform whatever sits at the
other end of the callback

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

[FernetMenta]

i.e DestRect https://github.com/FernetMenta/xbmc/blob/master/xbmc/cores/IPlayer.h#L113

[FernetMenta]

@topfs2 btw:

Feels very hacky to have players call directly into the info manager IMO.

that's the case already for dvdplayer: https://github.com/xbmc/xbmc/blob/master/xbmc/cores/dvdplayer/DVDPlayer.cpp#L2264
(not the only pos)
don't get me wrong. I am not saying that this is nice.

[FernetMenta]

here is an alternative: FernetMenta@65db4d9

it's the first step of what I think is a better solution. all data accessed by gui and external clients shoud stay in this cache and finally the polling methods from the interfaces will disappear.

[topfs2]

Dangit, didn't realize we needed the update for dest rect. And that we already had player call into info manager already. Considering those I have nothing against the patch.

Regarding the alternative. I'm a bit tired so haven't looked to hard at it but seems like the one in this PR is good enough, and the other a bit to much perhaps? But could be that I'm just tired. Which ever you prefer is good by me.

[fritsch]

I like the alternative more. It goes more into the direction we discussed by mail and could be extended in the future and decouples our players again.

[FernetMenta]

@topfs2 I switched this to the alternative which is in this state basically the same as the original PR with the difference that that the update flag and trigger are in a separate class. have a relaxed evening and maybe you could have another look at it tomorrow

jenkins build this please

[fritsch]

This needs Xcode changes, right?

[herrnst]

@fritsch @FernetMenta gave the latest approach a try, labels can still be hammered, streaminfo (channel count on ac3 property change) updates inbetween, everything looking as usual, but didn't test that extensively, though. But looks good so far.

[Memphiz]

pr with xcode sync sent to fernetmenta ... just keep forgetting me ... ;)

[FernetMenta]

jenkins build this please

[fritsch]

build failure looks like a jenkins issue

[FernetMenta]

jenkins build this please