refactor dvdplayervideo demux queue to include codec buffering #835

Merged
merged 1 commit into from Apr 4, 2012

Conversation

Projects
None yet
4 participants
Contributor

davilla commented Mar 31, 2012

This extends the dvdplayervideo demux queue to include possible codec buffering. Some hw codecs run external buffers and dvdplayer/dvdplayervideo needs to include these buffers in making decisions during video startup/playback. Right now we attempt to keep external buffing to a min and pray that we do not cause too much thrashing. This refactor eliminates these games.

This is slightly tricky as the demux queue can be used by size or by time and you have to handle both cases as you do not know which is being used.

  1. move dvdplayer demux message queue private and add accessors.
  2. change accessors to better indicate their functions and remove unused functions.
  3. extend queues to include possible codec buffering.

@ghost ghost assigned elupus Mar 31, 2012

Member

jmarshallnz commented Mar 31, 2012

Changes look sane to me - assigning to @elupus

Member

elupus commented Apr 1, 2012

Sane idea. Some things. Could you avoid the rename of AcceptsData() to IsFull(). Also split out the removal of WaitForBuffer(). I thought it was used, but seems it's not. There are some cosmetics in there too (rename of m_iMaxDataSize )

Contributor

davilla commented Apr 2, 2012

changed and force pushed.

@jmarshallnz, I did not address the const question, the originals are not const so I matches them less I get ponked by the dvdplayer master.
@elupus, WaitForBuffer restored, not used in xbmc. I reverted the change AcceptsData -> IsFull but think about this one, 90 percent usage is !AcceptsData which means a double ! as AcceptsData is !IsFull :)

Member

jmarshallnz commented Apr 2, 2012

Seems to me that the 3 new functions can be (and given their names should be) const, but it's @elupus domain so will yield to him - no big deal anyway!

Contributor

davilla commented Apr 3, 2012

ping elupus, your call on const.

Member

elupus commented Apr 3, 2012

Const the shit out of it. :-)

Contributor

davilla commented Apr 3, 2012

jmarshallnz, const help on this. I'm not sure when one should or should not declare a func const.

Member

jmarshallnz commented Apr 3, 2012

I think all 4 of these should be const'able (heh):
{{{
bool AcceptsData() { return !m_messageQueue.IsFull(); }
bool HasData() { return m_messageQueue.GetDataSize() > 0; }
int GetLevel() { return m_messageQueue.GetLevel(); }
bool IsInited() { return m_messageQueue.IsInited(); }
}}}
All the functions they call are already const.

@jmarshallnz jmarshallnz closed this Apr 3, 2012

@jmarshallnz jmarshallnz reopened this Apr 3, 2012

Contributor

davilla commented Apr 3, 2012

constfied

Contributor

davilla commented Apr 4, 2012

missed one.

I'd really like this pull/req not to turn into a bikeshed about const, that's not the point of it and why I did not have them in the 1st place.

Contributor

davilla commented Apr 4, 2012

inject in the morning unless there are any objections.

@ghost ghost assigned davilla Apr 4, 2012

Member

jmarshallnz commented Apr 4, 2012

go for it.

Member

elupus commented Apr 4, 2012

Yup go ahead. While I don't really like the time vs size handling I don't
know of a better solution right now.
On Apr 4, 2012 5:32 AM, "jmarshallnz" <
reply@reply.github.com>
wrote:

go for it.


Reply to this email directly or view it on GitHub:
#835 (comment)

davilla added a commit that referenced this pull request Apr 4, 2012

Merge pull request #835 from davilla/dvdplayervideo-demux-queue-refactor
refactor dvdplayervideo demux queue to include codec buffering

@davilla davilla merged commit ac897dc into xbmc:master Apr 4, 2012

+ double timesize = m_messageQueue.GetTimeSize();
+ if (m_pVideoCodec)
+ timesize += m_pVideoCodec->GetTimeSize();
+ return min(100, MathUtils::round_int(100.0 * m_messageQueue.GetMaxTimeSize() * timesize));
@IDisposable

IDisposable May 17, 2012

Isn't range 0.0 <= timesize <= m_messageQueue.GetMaxTimeSize()? If so, then a proper percentage calculation (mirroring what is done for IsDataBased elements) should be:

return min(100, MathUtils::round_int(100.0 * timesize / m_messageQueue.GetMaxTimeSize()));
@davilla

davilla May 17, 2012

Contributor

See m_messageQueue.SetMaxTimeSize

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment