do not access m_pVideoCodec from dvdplayer thread

[Jalle19]

This fixes access violations from time to time when switching channels using DMX_STREAM_CHANGE packets. This also aligns the CDVDPlayerVideo implementation with CDVDPlayerAudio.

Adding the backport label, @FernetMenta please remove if in your opinion this is too uncertain.

[FernetMenta]

+1 for master
backport needs testing with AML codec which is the only one which has implemented this IMO nonsense.

@Jalle19 can you submit another PR with removal of now obsolete methods Codec::GetDataSize and GetTimeSize ?

[Jalle19]

Since level (which is what GetLevel() returns) wasn't touched at all by the rest of the method's code I think this is safe regardless of platform.

[FernetMenta]

check the (ugly) code you removed. It does not only return level.

[Jalle19]

@fritsch did you have any concerns?

[fritsch]

From your backtrace - this does not fix your issue.

[Jalle19]

@fritsch the trace in the e-mail? The code that caused the problem is removed and I didn't manage to trigger it with this patch.

[fritsch]

Okay - you linked me some other lines on IRC which mislead me. After looking again through the backtrace you sent by mail, this will fix it for you.

Thanks much. In with it.

[Jalle19]

@fritsch the IRC thing was a conpletely unrelated thing I found will looking through some crash logs.

[Jalle19]

jenkins build this please

[FernetMenta]

arg, accidentally deleted my last comment with this tablet keyboard :)
I mean the methods of aml codec should be removed as well, even build succeeds

[fritsch]

AMLCodec needs rework for this. As those methods are massively used in critical parts to keep the hw decoder filled.

[davilla]

Yep, Good luck when dvdplayer has no knowledge anymore of how much is in the aml's hw codec internal buffers :) I suggest git blame to learn the reason why these methods were added to support hw codecs with large internal buffers.

[FernetMenta]

dvdplayer does not want to know anything about aml's internal buffers. @davilla please let us know why you have implemented this?

[fritsch]

@davilla as you just pointed out on IRC that you fixed that in a proper way - mind linking the commits?

[FernetMenta]

@fritsch if he did not remove those methods, it is not fixed properly

[fritsch]

@FernetMenta we don't know - but without knowing, we can only speculate - so let's wait and see and perhaps find a good way to fix it properly.

[FernetMenta]

I do know that this is wrong and does not fit into dvdplayer's architecture. For whatever reason it was done, it is not right.

[fritsch]

That can be clearly seen, yes. But let's wait on @davilla reply to understand his reasoning and work together on a fix. If the problem can be understood a proper fix can be made ...

[davilla]

dvdplayer uses the state the demux buffers to make decisions about playback. If those are drained or close to empty, it will (among other things) stop playback to buffer. I call it 'thrashing'. Now if aml's hw codec sucks down several seconds out of dvdplayer's buffers, dvdplayer can stop prematuraly or start thrashing about. AML's hw buffers are big, they can be several 10s of seconds in size. aml codec is not like ffmpeg where you always get a pict for each demux, it's a streaming device that needs a constant flow of demux packets or it starts thrashing. The trick is to keep it feed yet not let it drain dvdplayers demux buffers. You cannot just feed one demux and expect one frame out. Remember that is also is bypass in renderer.

@fritsch, the fix mentioned involves using newer aml codec functions to get a much better estimate on the amount of 'time' in the aml hw buffers and to use this to try and keep that buffering between 0.5 and 2.0 seconds. The current code tries to use dts/pts timestamps but this can get problematic as you really want the timestamp from the codec not the demux. Why 2.0 seconds, because when you have frames that don't change much, 2 seconds is not very much demux in bytes and that can drain fast. Then aml hw codec starts thrashing when it's buffer is empty. This fix still uses GetDataSize and GetDataTime.

No other codec except maybe android mediacodec and the broadcom crystal HD work this way. Crystal HD is more similar, while mediacodec can only suck down 8-16 frames of demux before outputting a picture. Crystal HD is dead so that leaves amlcodec but dvdplayer would benefit from knowning how much is internally buffered by mediacodec.

This fix was blessed by @elupus who was instrumental in the design of the dvdplayer architecture. And he agreed that dvdplayer really does need to know codec buffering state as it makes decisions based on demux buffering state. Normally it does not matter much except when the codec internally buffers more than about 1/2 second. At one point we talked about adding the ability for the codec to hold the demux packets instead of dvdplayervideo. That way, the codec could feed when it wanted and not just when dvdplayervideo cycled through it's process loop.

If you remove this, aml codec will become useless and you might as well just rm it. While my fix helps in controlling the amount the codec buffers, there are still up to 2 seconds pulled out of dvdplayervideo demux buffer. This means videos will end 2 seconds early and buffering can start 2 seconds earlier.

[FernetMenta]

You cannot just feed one demux and expect one frame out

if you think that this has ever been a requirement, your are wrong. vdpau, vaapi may not use such big internal buffers but they control their buffers by themselves. a codec can request a buffer and/or signal a picture, or just do nothing.

If you remove this, aml codec will become useless and you might as well just rm it

nobody said this should be removed without a proper alternative.

[davilla]

Based on years of bringing in non-ffmpeg streaming codecs into dvdplayer, I beg to differ :) I highly doubt that vdpau/vaapi buffer anymore demux that is required for the format's profile. And remember that both vdpau/vaapi know the internal details of the format while non-ffmpeg codecs do not.

Maybe you know a better way.

[FernetMenta]

Maybe you know a better way

Certainly I do and I try not to ignore basic OO principles like information hiding and encapsulation.

I highly doubt that vdpau/vaapi buffer anymore demux that is required for the format's profile

They don't but they easily could. They are kind of similar to ALM in having an embedded thread.

[fritsch]

Can we find a solution so that AML and other codecs get the info they need out of dvdplayer without accessing the internal members directly?

So a solution that does not force us to rewrite all of them, but to extend dvdplayer interface to support those (may be in your eyes legacy) codecs?

Or in other words: What is the easiest way to solve this a "step" better while fixing the original bug, but keep them working?

[davilla]

Here's a good example of 'feed one demux and expect one frame out'.

When dvdplayervideo calls the demux method, you MUST return buffer and/or picture or error. You MUST do something with the demux packet as there is no 'busy, feed me later'. If you don't save it or pass it down, then that demux packet is lost. If dvdplayervideo forces the codec to eat demux packets, then there must be a method to know how much might be buffered and add that to the amount that dvdplayer then used to make decisions. Otherwise, dvdplayer is not playing with a complete information.

Without the knowledge of codec buffering, the method only works if and only if there is a picture available within the codec internal buffering bounds. If the internal buffer is full, packets can be lost. pictures come out pretty quick under ffmpeg/vdpau/vaapi. Maybe a few cycles of 'buffer' before picts start coming out.

AML codec is a streaming codec, it takes some time for pictures to come out. Spectially on startup, the pathway is long. Such codecs are very hard to deal with under dvdplayervideo because of it's 'feed one demux and expect one frame out' design. Maybe I should say 'feed one demux and expect one frame out really soon'.

[FernetMenta]

You do not need to tell me how this works. I do know dvdplayer much better than you do.

dvdplayer uses the state the demux buffers to make decisions about playback

write those down and you will see that this is not much.

a codec requests packets from the player so player never comes into the situation that it pulls a demux packet from the queue and can't deliver it.

[davilla]

Humm, I only spent +12 years dealing with dvdplayer on an intimate basis, bringing in several streaming hw codecs.

Ok, your ball then. good luck.

[fritsch]

https://en.wikipedia.org/dvdplayer-syndrome <- needs updating ...

I am out here. Sorry @davilla for wasting your time.

[Jalle19]

@FernetMenta I'm sure both you and @davilla know dvdplayer very well. This is not a contest, this discussion should be about how to fix this bug.

Now, I don't have any AML devices so I can't confirm whether this messes anything up for those. If @davilla is right and we need this code then maybe we can protect m_pVideoCodec with a lock or something?

[FernetMenta]

Humm, I only spent +12 years dealing with dvdplayer on an intimate basis

History. I haven't seen a single line of code of you contributing to this project in more than x years. Don't try protecting old code, try to improve things. I don't deny that this code did anything good but that does not change the fact that it is not ideal. Instead of trying to protect an old code base you should contribute in making things better.
Maintaining a hidden repository, take things for your benefits, and don't give anything back is bad style I really dislike.

[davilla]

Nice attitude. I rescued osx from death when plex spun off, brought in atv, ios, and android platforms. Codecs list includes chd, vtb, vda, mediacodec, amcodec. AMLPlayer too (which was replaced by amcodec). Created the depends concept. Just to name a few things. My commit count over the years to date is still in the top one percent. Don't even suggest I never give back, that only illustrates how clueless you are. It's attitude like that keep me away from projects like this and one of the reasons I left as an xbmc dev in the 1st place. No respect for the history and work of others.

Lets do the stats;
936 Rainer Hochecker fernetmenta@online.de

vs

802 davilla davilla@svn
739 davilla davilla@4pi.com
423 S. Davilla davilla@4pi.com

I've given plenty back over the years. History counts but not in the open source world it seems.

Now I'm really out, serves me right for dipping a toe in shark infested waters. I'll not make the same mistake again. Again good luck with the refactor of amcodec and friends.

[FernetMenta]

@Jalle19 As I mentioned earlier the assumption taken in aml codec that it needs to return something is wrong. It is fine to return nothing.

  // we must return VC_BUFFER or VC_PICTURE,
  // default to VC_BUFFER.
  int rtn = VC_BUFFER;
  if (m_old_pictcnt != m_cur_pictcnt)

Can you pick this commit to this pr? It prevents aml codec from draining player's message queue
FernetMenta@3d31091

[Jalle19]

@FernetMenta done, @stefansaraev reported things as still working

[FernetMenta]

Great. I suggest to merge it. It fixes a segfault

[Jalle19]

jenkins build and merge

@FernetMenta what about backporting?

[Jalle19]

It's a bit hard to tell how many have been affected by this since Kodi doesn't crash, playback merely stops.

[FernetMenta]

@Jalle19 imo a fixed segfault justifies a backport

[Jalle19]

jenkins build this please again

[MartijnKaijser]

ignore build error

[MartijnKaijser]

@Jalle19 still needs to be done as backport PR am I correct?

[Jalle19]

#7659