CR-Client: change base video renderer decoder thread polling behavior

Description
-----------------------------------------
When we decode on a separate thread in the base video
renderer,
we previously set up an IHXPaceMaker and had it call us
back to our OnPace() method regularly every 5ms. Most of the
time,
of course, there are no frames to decode and so the call
to OnPace() does nothing. However, this constant sleeping
and waking of the internal IHXPaceMaker thread results
in a lot of power consumption on mobile devices, as
reported
by Intel.

Therefore the goal of this change is to wake the decoder
thread
up as few times as possible, and only when there is encoded
frames to decode and space in the decoded frame output
queue.

To accomplish this, we run the decoder pacemaker in
suspended
mode all the time (using the new suspend internal behavior
of
CVideoPaceMaker). Therefore in suspended mode, the decode
thread
will be asleep until we signal it. When we signal it, we
will get
one callback to our OnPace method and then the thread will
go
back to sleep.

Currently, each time we get an OnPace call, the base video
renderer
decodes as many frames as it can. It will decode frames
until
either: a) it has exhausted the input queue of encoded
frames; or
b) it has exhausted the space in the decoded frame output
queue.

Therefore, we signal the decoder thread if either one
of these conditions occur:

1) if an assembled encoded frame was just added to the input
queue
   AND there is currently space in the output queue;
2) if we just removed a decoded frame from a FULL output
queue.

The normal behavior then is that we are waking the thread
up
pretty often at the beginning until the output queue is
filled
up. By this time the input queue usually holds a lot of
input
encoded frames. After the output queue is filled up, then
we
wake up the decoder thread only as needed.

Also, there were a few places (like when we were paused)
that the base video renderer suspended the decode
pacemaker.
Since we are ALWAYS running the decode pacemaker in
suspended
mode, then there is no need to make these calls.

Files Modified
-----------------------------------------
datatype/common/vidrend/vidrend.cpp
datatype/common/vidrend/vidrendf.cpp
datatype/common/vidrend/pub/vidrend.h

Branches
-----------------------------------------
HEAD and hxclient_3_1_0_atlas

Testing
-----------------------------------------
I built all the vidrend-derived renderers and played
back video using the RealVideo, Windows Media Video,
and MPEG4 video renderers. I tested normal playback
and seeking using splay. I also verified from logs
that all frames were decoded and bltted.

=====================================
Eric Hyche, Technical Lead
RealNetworks, Inc.
ehychereal.com

_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

Eric Hyche wrote:
> Description
> -----------------------------------------
> When we decode on a separate thread in the base video
renderer,
> we previously set up an IHXPaceMaker and had it call
us
> back to our OnPace() method regularly every 5ms. Most
of the time,
> of course, there are no frames to decode and so the
call
> to OnPace() does nothing. However, this constant
sleeping
> and waking of the internal IHXPaceMaker thread results
> in a lot of power consumption on mobile devices, as
reported
> by Intel.
> 
> Therefore the goal of this change is to wake the
decoder thread
> up as few times as possible, and only when there is
encoded
> frames to decode and space in the decoded frame output
queue.
> 
> To accomplish this, we run the decoder pacemaker in
suspended
> mode all the time (using the new suspend internal
behavior of
> CVideoPaceMaker). Therefore in suspended mode, the
decode thread
> will be asleep until we signal it. When we signal it,
we will get
> one callback to our OnPace method and then the thread
will go
> back to sleep.
> 
> Currently, each time we get an OnPace call, the base
video renderer
> decodes as many frames as it can. It will decode frames
until
> either: a) it has exhausted the input queue of encoded
frames; or
> b) it has exhausted the space in the decoded frame
output queue.
> 
> Therefore, we signal the decoder thread if either one
> of these conditions occur:
> 
> 1) if an assembled encoded frame was just added to the
input queue
>    AND there is currently space in the output queue;
> 2) if we just removed a decoded frame from a FULL
output queue.

2) ...and there are encoded frames to decode?


rest looks good.
--greg.


> 
> The normal behavior then is that we are waking the
thread up
> pretty often at the beginning until the output queue is
filled
> up. By this time the input queue usually holds a lot of
input
> encoded frames. After the output queue is filled up,
then we
> wake up the decoder thread only as needed.
> 
> Also, there were a few places (like when we were
paused)
> that the base video renderer suspended the decode
pacemaker.
> Since we are ALWAYS running the decode pacemaker in
suspended
> mode, then there is no need to make these calls.
> 
> Files Modified
> -----------------------------------------
> datatype/common/vidrend/vidrend.cpp
> datatype/common/vidrend/vidrendf.cpp
> datatype/common/vidrend/pub/vidrend.h
> 
> Branches
> -----------------------------------------
> HEAD and hxclient_3_1_0_atlas
> 
> Testing
> -----------------------------------------
> I built all the vidrend-derived renderers and played
> back video using the RealVideo, Windows Media Video,
> and MPEG4 video renderers. I tested normal playback
> and seeking using splay. I also verified from logs
> that all frames were decoded and bltted.
> 
> =====================================
> Eric Hyche, Technical Lead
> RealNetworks, Inc.
> ehychereal.com
> 
> 
>
------------------------------------------------------------
------------
> 
> _______________________________________________
> Datatype-dev mailing list
> Datatype-devhelixcommunity.org
> http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev


_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

> -----Original Message-----
> From: Greg Wright [mailto:gwrightreal.com] 
> Sent: Friday, April 18, 2008 4:04 PM
> To: ehychereal.com
> Cc: datatype-devlists.helixcommunity.org
> Subject: Re: [datatype-dev] CR-Client: change base
video 
> renderer decoder thread polling behavior
> 
> Eric Hyche wrote:
> > Description
> > -----------------------------------------
> > When we decode on a separate thread in the base
video renderer,
> > we previously set up an IHXPaceMaker and had it
call us
> > back to our OnPace() method regularly every 5ms.
Most of the time,
> > of course, there are no frames to decode and so
the call
> > to OnPace() does nothing. However, this constant
sleeping
> > and waking of the internal IHXPaceMaker thread
results
> > in a lot of power consumption on mobile devices,
as reported
> > by Intel.
> > 
> > Therefore the goal of this change is to wake the
decoder thread
> > up as few times as possible, and only when there
is encoded
> > frames to decode and space in the decoded frame
output queue.
> > 
> > To accomplish this, we run the decoder pacemaker
in suspended
> > mode all the time (using the new suspend internal
behavior of
> > CVideoPaceMaker). Therefore in suspended mode, the
decode thread
> > will be asleep until we signal it. When we signal
it, we will get
> > one callback to our OnPace method and then the
thread will go
> > back to sleep.
> > 
> > Currently, each time we get an OnPace call, the
base video renderer
> > decodes as many frames as it can. It will decode
frames until
> > either: a) it has exhausted the input queue of
encoded frames; or
> > b) it has exhausted the space in the decoded frame
output queue.
> > 
> > Therefore, we signal the decoder thread if either
one
> > of these conditions occur:
> > 
> > 1) if an assembled encoded frame was just added to
the input queue
> >    AND there is currently space in the output
queue;
> > 2) if we just removed a decoded frame from a FULL
output queue.
> 
> 2) ...and there are encoded frames to decode?
> 

Well, checking that would require grabbing the mutex
protecting
the input queue, and I wanted to avoid doing that for risk
of deadlock. Given that the input queue generally remains 
pretty full almost all the time, avoiding the input queue
check only results in a few extra checks at the very end
of playback (on the order of 10 extra wakeups total over
the entire playback).

This change already reduces the number of thread wakeups
from roughly 200/second (we were asking for the thread to
call us back every 5ms) to about 23/second - about a 90%
reduction.

Eric

> 
> rest looks good.
> --greg.
> 
> 
> > 
> > The normal behavior then is that we are waking the
thread up
> > pretty often at the beginning until the output
queue is filled
> > up. By this time the input queue usually holds a
lot of input
> > encoded frames. After the output queue is filled
up, then we
> > wake up the decoder thread only as needed.
> > 
> > Also, there were a few places (like when we were
paused)
> > that the base video renderer suspended the decode
pacemaker.
> > Since we are ALWAYS running the decode pacemaker
in suspended
> > mode, then there is no need to make these calls.
> > 
> > Files Modified
> > -----------------------------------------
> > datatype/common/vidrend/vidrend.cpp
> > datatype/common/vidrend/vidrendf.cpp
> > datatype/common/vidrend/pub/vidrend.h
> > 
> > Branches
> > -----------------------------------------
> > HEAD and hxclient_3_1_0_atlas
> > 
> > Testing
> > -----------------------------------------
> > I built all the vidrend-derived renderers and
played
> > back video using the RealVideo, Windows Media
Video,
> > and MPEG4 video renderers. I tested normal
playback
> > and seeking using splay. I also verified from
logs
> > that all frames were decoded and bltted.
> > 
> > =====================================
> > Eric Hyche, Technical Lead
> > RealNetworks, Inc.
> > ehychereal.com
> > 
> > 
> > 
>
------------------------------------------------------------
--
> ----------
> > 
> > _______________________________________________
> > Datatype-dev mailing list
> > Datatype-devhelixcommunity.org
> > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
> 


_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

>>> 1) if an assembled encoded frame was just added
to the input queue
>>>    AND there is currently space in the output
queue;
>>> 2) if we just removed a decoded frame from a
FULL output queue.
>> 2) ...and there are encoded frames to decode?
>>
> 
> Well, checking that would require grabbing the mutex
protecting
> the input queue, and I wanted to avoid doing that for
risk
> of deadlock. Given that the input queue generally
remains 
> pretty full almost all the time, avoiding the input
queue
> check only results in a few extra checks at the very
end
> of playback (on the order of 10 extra wakeups total
over
> the entire playback).
> 
> This change already reduces the number of thread
wakeups
> from roughly 200/second (we were asking for the thread
to
> call us back every 5ms) to about 23/second - about a
90%
> reduction.

sounds good.
--greg.

> 
> Eric

_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

Milko,

Thanks for the review. Comments inline below...

> -----Original Message-----
> From: Milko Boic [mailto:milkoreal.com] 
> Sent: Monday, April 21, 2008 4:09 PM
> To: ehychereal.com; 'Greg Wright'
> Cc: datatype-devlists.helixcommunity.org
> Subject: RE: [datatype-dev] CR-Client: change base
video 
> renderer decoder thread polling behavior
> 
> 
> Two comments:
> 
> 1.) I would eliminate the check for FULL queue on
retrieval 
> from the post-decode (output) queue and Signal in all
cases 
> upon retrieval from the output queue.  The reason is
that 
> current logic creates a potential deadlock condition:
>           - out queue is non-full
>         - decoder runs and fills-up the out queue
>           - new packets arrive but decoder is not
signaled 
> since out-queue is full
>         - renderer retrieves frame from the out queue
>         - renderer does not signal the decoder since
out 
> queue wasn't full
> 
>         The above situation will result in newly
arrived 
> packets not getting decoded until another packet
arrives.  
> This could be late or never.
> 
>         I suggest to always signal the decoder on
retrieved 
> frame from the out queue.  Given that input queue is
full in 
> normal cases, this will result in useful signalling in
most cases.
> 

Sounds good - I will make this change.

> 
> 2.) When entering seek, the removed code was making 
> preventing the decoder from using the CPU for pre-seek
frames 
> thus improving the responsiveness of the seek
operation.  We 
> need to keep this functionality.  Since decoder is now

> signaled causally, we cannot just rely on suspending
the 
> thread but we must also instruct the decoder not to
decode 
> when invoked.

Ok, I see the logic for adding
m_pVideoFormat->SuspendDecode(TRUE)
in OnPreSeek() and m_pVideoFormat->SuspendDecode(FALSE)
in
OnPostSeek(). This will prevent us from actually doing any
decoding if we get any packets we receive in between
OnPreSeek()
and OnPostSeek().

In OnPreSeek(), however, if we call 
m_pVideoFormat->SuspendDecode(TRUE),
then there is no need for this:

>     // Suspend the Decoder pump
>     if (m_pDecoderPump)
>     {
>         m_pDecoderPump->Suspend(TRUE);
>         m_pDecoderPump->Signal();
>         m_pDecoderPump->WaitForSuspend();
>     }
> 

since it has no effect at all. The call to
m_pVideoFormat->SuspendDecode(TRUE)
sets CVideoFormat::m_bDecodeSuspended to TRUE, and in the
loop
logic in CVideoFormat:ecodeFra
me(), if m_bDecodeSuspended is TRUE,
then CVideoFormat:ecodeFra
me() is guaranteed to not do any decoding.

In OnPostSeek(), I agree we need to call
m_pVideoFormat->SuspendDecode(FALSE),
but we definitely do NOT want to do this:

>     if (m_pDecoderPump)
>     {
>         m_pDecoderPump->Suspend(FALSE);
>         m_pDecoderPump->Signal();
>     }

since this will take the decoder pacemaker out of suspended
mode
and return it to making calls every DECODE_INTEVAL ms. We
want
to run the decoder pacemaker in suspended mode all
the time.

=============================================
Eric Hyche (ehychereal.com)
Technical Lead
RealNetworks, Inc. 

> Suspending and waiting for decoder pump to suspend is
still 
> useful to sync the decoded suspension state and thus
make 
> sure that flush of the queues is complete.
> 
> STDMETHODIMP CVideoRenderer::OnPreSeek(ULONG32
ulOldTime, 
> ULONG32 ulNewTime)
> {
>     HXLOGL3(HXLOG_BVID, 
> "CVideoRenderer::OnPreSeek(OldTime=%lu,
NewTime=%lu)", 
> ulOldTime, ulNewTime);
> 
> +   m_pVideoFormat->SuspendDecode(TRUE);
> 
>     // Change state to stop Blts
>     m_pMutex->Lock();
>     EndBuffering();
>     m_PlayState = Seeking;
>     m_pMutex->Unlock();
> 
>     // Suspend the Decoder pump
>     if (m_pDecoderPump)
>     {
>         m_pDecoderPump->Suspend(TRUE);
>         m_pDecoderPump->Signal();
>         m_pDecoderPump->WaitForSuspend();
>     }
> 
>     // Wait for Blt in progress to complete and reset
>     // packet queues
>     DisplayMutex_Lock();
> 
> STDMETHODIMP CVideoRenderer::OnPostSeek(ULONG32
ulOldTime, 
> ULONG32 ulNewTime)
> {
>     HXLOGL3(HXLOG_BVID, 
> "CVideoRenderer::OnPostSeek(OldTime=%lu,
NewTime=%lu)", 
> ulOldTime, ulNewTime);
> 
>     DisplayMutex_Lock();
> 
> +  m_pVideoFormat->SuspendDecode(FALSE);
> 
>     if (m_pDecoderPump)
>     {
>         m_pDecoderPump->Suspend(FALSE);
>         m_pDecoderPump->Signal();
>     }
> 
>     if (m_bUseVideoSurface2 && m_pMISUSSite)
>     {
>         FlushVideoSurface2(m_pMISUSSite);
>     }
> 
> 
> 
> At 01:16 PM 4/18/2008, Eric Hyche wrote:
> 
> 
> 
> 	> -----Original Message-----
> 	> From: Greg Wright [ mailto:gwrightreal.com

> <mailto:gwrightreal.com> ] 
> 	> Sent: Friday, April 18, 2008 4:04 PM
> 	> To: ehychereal.com
> 	> Cc: datatype-devlists.helixcommunity.org
> 	> Subject: Re: [datatype-dev] CR-Client: change
base video 
> 	> renderer decoder thread polling behavior
> 	> 
> 	> Eric Hyche wrote:
> 	> > Description
> 	> > -----------------------------------------
> 	> > When we decode on a separate thread in the
base 
> video renderer,
> 	> > we previously set up an IHXPaceMaker and had
it call us
> 	> > back to our OnPace() method regularly every
5ms. 
> Most of the time,
> 	> > of course, there are no frames to decode and
so the call
> 	> > to OnPace() does nothing. However, this
constant sleeping
> 	> > and waking of the internal IHXPaceMaker
thread results
> 	> > in a lot of power consumption on mobile
devices, as reported
> 	> > by Intel.
> 	> > 
> 	> > Therefore the goal of this change is to wake
the 
> decoder thread
> 	> > up as few times as possible, and only when
there is encoded
> 	> > frames to decode and space in the decoded
frame 
> output queue.
> 	> > 
> 	> > To accomplish this, we run the decoder
pacemaker in 
> suspended
> 	> > mode all the time (using the new suspend
internal 
> behavior of
> 	> > CVideoPaceMaker). Therefore in suspended
mode, the 
> decode thread
> 	> > will be asleep until we signal it. When we
signal 
> it, we will get
> 	> > one callback to our OnPace method and then
the 
> thread will go
> 	> > back to sleep.
> 	> > 
> 	> > Currently, each time we get an OnPace call,
the 
> base video renderer
> 	> > decodes as many frames as it can. It will
decode 
> frames until
> 	> > either: a) it has exhausted the input queue
of 
> encoded frames; or
> 	> > b) it has exhausted the space in the decoded
frame 
> output queue.
> 	> > 
> 	> > Therefore, we signal the decoder thread if
either one
> 	> > of these conditions occur:
> 	> > 
> 	> > 1) if an assembled encoded frame was just
added to 
> the input queue
> 	> >    AND there is currently space in the
output queue;
> 	> > 2) if we just removed a decoded frame from a
FULL 
> output queue.
> 	> 
> 	> 2) ...and there are encoded frames to decode?
> 	> 
> 	
> 	Well, checking that would require grabbing the mutex
protecting
> 	the input queue, and I wanted to avoid doing that for
risk
> 	of deadlock. Given that the input queue generally
remains 
> 	pretty full almost all the time, avoiding the input
queue
> 	check only results in a few extra checks at the very
end
> 	of playback (on the order of 10 extra wakeups total
over
> 	the entire playback).
> 	
> 	This change already reduces the number of thread
wakeups
> 	from roughly 200/second (we were asking for the thread
to
> 	call us back every 5ms) to about 23/second - about a
90%
> 	reduction.
> 	
> 	Eric
> 	
> 	> 
> 	> rest looks good.
> 	> --greg.
> 	> 
> 	> 
> 	> > 
> 	> > The normal behavior then is that we are
waking the thread up
> 	> > pretty often at the beginning until the
output 
> queue is filled
> 	> > up. By this time the input queue usually
holds a 
> lot of input
> 	> > encoded frames. After the output queue is
filled up, then we
> 	> > wake up the decoder thread only as needed.
> 	> > 
> 	> > Also, there were a few places (like when we
were paused)
> 	> > that the base video renderer suspended the
decode pacemaker.
> 	> > Since we are ALWAYS running the decode
pacemaker in 
> suspended
> 	> > mode, then there is no need to make these
calls.
> 	> > 
> 	> > Files Modified
> 	> > -----------------------------------------
> 	> > datatype/common/vidrend/vidrend.cpp
> 	> > datatype/common/vidrend/vidrendf.cpp
> 	> > datatype/common/vidrend/pub/vidrend.h
> 	> > 
> 	> > Branches
> 	> > -----------------------------------------
> 	> > HEAD and hxclient_3_1_0_atlas
> 	> > 
> 	> > Testing
> 	> > -----------------------------------------
> 	> > I built all the vidrend-derived renderers
and played
> 	> > back video using the RealVideo, Windows
Media Video,
> 	> > and MPEG4 video renderers. I tested normal
playback
> 	> > and seeking using splay. I also verified
from logs
> 	> > that all frames were decoded and bltted.
> 	> > 
> 	> > =====================================
> 	> > Eric Hyche, Technical Lead
> 	> > RealNetworks, Inc.
> 	> > ehychereal.com
> 	> > 
> 	> > 
> 	> > 
> 	>
------------------------------------------------------------
--
> 	> ----------
> 	> > 
> 	> >
_______________________________________________
> 	> > Datatype-dev mailing list
> 	> > Datatype-devhelixcommunity.org
> 	> > 
> http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev 
> <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev> 
> 	> 
> 	
> 	
> 	_______________________________________________
> 	Datatype-dev mailing list
> 	Datatype-devhelixcommunity.org
> 	
> http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev 
> <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>  
> 
> 
> 


_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

 >In OnPreSeek(), however, if we call 
m_pVideoFormat->SuspendDecode(TRUE),
 >then there is no need for this:
 >
 >     // Suspend the Decoder pump
 >     if (m_pDecoderPump)
 >     {
 >         m_pDecoderPump->Suspend(TRUE);
 >         m_pDecoderPump->Signal();
 >         m_pDecoderPump->WaitForSuspend();
 >     }
 >
 >
 >since it has no effect at all. The call to 
m_pVideoFormat->SuspendDecode(TRUE)
 >sets CVideoFormat::m_bDecodeSuspended to TRUE, and in
the loop
 >logic in CVideoFormat:ecodeFra
me(), if m_bDecodeSuspended is TRUE,
 >then CVideoFormat:ecodeFra
me() is guaranteed to not do any decoding.

It is Signal and WaitForSuspend that have the effect.  These
statements 
make sure that decoder has indeed made notice of
SuspendDecode() 
directive.  Thus, for statements that follow
WaitForSuspend() it is 
guaranteed that decoder will not be moving data from input
to output queue 
or operating on locally held data.  This is important to
make sure the 
decoder avoids the race condition with Reset in statements
below and 
thus  complete flush of the data is accomplished.

Milko


At 01:38 PM 4/21/2008, Eric Hyche wrote:

>Milko,
>
>Thanks for the review. Comments inline below...
>
> > -----Original Message-----
> > From: Milko Boic [mailto:milkoreal.com]
> > Sent: Monday, April 21, 2008 4:09 PM
> > To: ehychereal.com; 'Greg Wright'
> > Cc: datatype-devlists.helixcommunity.org
> > Subject: RE: [datatype-dev] CR-Client: change base
video
> > renderer decoder thread polling behavior
> >
> >
> > Two comments:
> >
> > 1.) I would eliminate the check for FULL queue on
retrieval
> > from the post-decode (output) queue and Signal in
all cases
> > upon retrieval from the output queue.  The reason
is that
> > current logic creates a potential deadlock
condition:
> >           - out queue is non-full
> >         - decoder runs and fills-up the out queue
> >           - new packets arrive but decoder is not
signaled
> > since out-queue is full
> >         - renderer retrieves frame from the out
queue
> >         - renderer does not signal the decoder
since out
> > queue wasn't full
> >
> >         The above situation will result in newly
arrived
> > packets not getting decoded until another packet
arrives.
> > This could be late or never.
> >
> >         I suggest to always signal the decoder on
retrieved
> > frame from the out queue.  Given that input queue
is full in
> > normal cases, this will result in useful
signalling in most cases.
> >
>
>Sounds good - I will make this change.
>
> >
> > 2.) When entering seek, the removed code was
making
> > preventing the decoder from using the CPU for
pre-seek frames
> > thus improving the responsiveness of the seek
operation.  We
> > need to keep this functionality.  Since decoder is
now
> > signaled causally, we cannot just rely on
suspending the
> > thread but we must also instruct the decoder not
to decode
> > when invoked.
>
>Ok, I see the logic for adding
m_pVideoFormat->SuspendDecode(TRUE)
>in OnPreSeek() and
m_pVideoFormat->SuspendDecode(FALSE) in
>OnPostSeek(). This will prevent us from actually doing
any
>decoding if we get any packets we receive in between
OnPreSeek()
>and OnPostSeek().
>
>In OnPreSeek(), however, if we call 
m_pVideoFormat->SuspendDecode(TRUE),
>then there is no need for this:
>
> >     // Suspend the Decoder pump
> >     if (m_pDecoderPump)
> >     {
> >         m_pDecoderPump->Suspend(TRUE);
> >         m_pDecoderPump->Signal();
> >         m_pDecoderPump->WaitForSuspend();
> >     }
> >
>
>since it has no effect at all. The call to
m_pVideoFormat->SuspendDecode(TRUE)
>sets CVideoFormat::m_bDecodeSuspended to TRUE, and in
the loop
>logic in CVideoFormat:ecodeFra
me(), if m_bDecodeSuspended is TRUE,
>then CVideoFormat:ecodeFra
me() is guaranteed to not do any decoding.
>
>In OnPostSeek(), I agree we need to call
m_pVideoFormat->SuspendDecode(FALSE),
>but we definitely do NOT want to do this:
>
> >     if (m_pDecoderPump)
> >     {
> >         m_pDecoderPump->Suspend(FALSE);
> >         m_pDecoderPump->Signal();
> >     }
>
>since this will take the decoder pacemaker out of
suspended mode
>and return it to making calls every DECODE_INTEVAL ms.
We want
>to run the decoder pacemaker in suspended mode all
>the time.
>
>=============================================
>Eric Hyche (ehychereal.com)
>Technical Lead
>RealNetworks, Inc.
>
> > Suspending and waiting for decoder pump to suspend
is still
> > useful to sync the decoded suspension state and
thus make
> > sure that flush of the queues is complete.
> >
> > STDMETHODIMP CVideoRenderer::OnPreSeek(ULONG32
ulOldTime,
> > ULONG32 ulNewTime)
> > {
> >     HXLOGL3(HXLOG_BVID,
> > "CVideoRenderer::OnPreSeek(OldTime=%lu,
NewTime=%lu)",
> > ulOldTime, ulNewTime);
> >
> > +   m_pVideoFormat->SuspendDecode(TRUE);
> >
> >     // Change state to stop Blts
> >     m_pMutex->Lock();
> >     EndBuffering();
> >     m_PlayState = Seeking;
> >     m_pMutex->Unlock();
> >
> >     // Suspend the Decoder pump
> >     if (m_pDecoderPump)
> >     {
> >         m_pDecoderPump->Suspend(TRUE);
> >         m_pDecoderPump->Signal();
> >         m_pDecoderPump->WaitForSuspend();
> >     }
> >
> >     // Wait for Blt in progress to complete and
reset
> >     // packet queues
> >     DisplayMutex_Lock();
> >
> > STDMETHODIMP CVideoRenderer::OnPostSeek(ULONG32
ulOldTime,
> > ULONG32 ulNewTime)
> > {
> >     HXLOGL3(HXLOG_BVID,
> > "CVideoRenderer::OnPostSeek(OldTime=%lu,
NewTime=%lu)",
> > ulOldTime, ulNewTime);
> >
> >     DisplayMutex_Lock();
> >
> > +  m_pVideoFormat->SuspendDecode(FALSE);
> >
> >     if (m_pDecoderPump)
> >     {
> >         m_pDecoderPump->Suspend(FALSE);
> >         m_pDecoderPump->Signal();
> >     }
> >
> >     if (m_bUseVideoSurface2 &&
m_pMISUSSite)
> >     {
> >         FlushVideoSurface2(m_pMISUSSite);
> >     }
> >
> >
> >
> > At 01:16 PM 4/18/2008, Eric Hyche wrote:
> >
> >
> >
> >       > -----Original Message-----
> >       > From: Greg Wright [ mailto:gwrightreal.com
> > <mailto:gwrightreal.com> ]
> >       > Sent: Friday, April 18, 2008 4:04 PM
> >       > To: ehychereal.com
> >       > Cc: datatype-devlists.helixcommunity.org
> >       > Subject: Re: [datatype-dev] CR-Client:
change base video
> >       > renderer decoder thread polling
behavior
> >       >
> >       > Eric Hyche wrote:
> >       > > Description
> >       > >
-----------------------------------------
> >       > > When we decode on a separate
thread in the base
> > video renderer,
> >       > > we previously set up an
IHXPaceMaker and had it call us
> >       > > back to our OnPace() method
regularly every 5ms.
> > Most of the time,
> >       > > of course, there are no frames to
decode and so the call
> >       > > to OnPace() does nothing. However,
this constant sleeping
> >       > > and waking of the internal
IHXPaceMaker thread results
> >       > > in a lot of power consumption on
mobile devices, as reported
> >       > > by Intel.
> >       > >
> >       > > Therefore the goal of this change
is to wake the
> > decoder thread
> >       > > up as few times as possible, and
only when there is encoded
> >       > > frames to decode and space in the
decoded frame
> > output queue.
> >       > >
> >       > > To accomplish this, we run the
decoder pacemaker in
> > suspended
> >       > > mode all the time (using the new
suspend internal
> > behavior of
> >       > > CVideoPaceMaker). Therefore in
suspended mode, the
> > decode thread
> >       > > will be asleep until we signal it.
When we signal
> > it, we will get
> >       > > one callback to our OnPace method
and then the
> > thread will go
> >       > > back to sleep.
> >       > >
> >       > > Currently, each time we get an
OnPace call, the
> > base video renderer
> >       > > decodes as many frames as it can.
It will decode
> > frames until
> >       > > either: a) it has exhausted the
input queue of
> > encoded frames; or
> >       > > b) it has exhausted the space in
the decoded frame
> > output queue.
> >       > >
> >       > > Therefore, we signal the decoder
thread if either one
> >       > > of these conditions occur:
> >       > >
> >       > > 1) if an assembled encoded frame
was just added to
> > the input queue
> >       > >    AND there is currently space in
the output queue;
> >       > > 2) if we just removed a decoded
frame from a FULL
> > output queue.
> >       >
> >       > 2) ...and there are encoded frames to
decode?
> >       >
> >
> >       Well, checking that would require grabbing
the mutex protecting
> >       the input queue, and I wanted to avoid doing
that for risk
> >       of deadlock. Given that the input queue
generally remains
> >       pretty full almost all the time, avoiding
the input queue
> >       check only results in a few extra checks at
the very end
> >       of playback (on the order of 10 extra
wakeups total over
> >       the entire playback).
> >
> >       This change already reduces the number of
thread wakeups
> >       from roughly 200/second (we were asking for
the thread to
> >       call us back every 5ms) to about 23/second -
about a 90%
> >       reduction.
> >
> >       Eric
> >
> >       >
> >       > rest looks good.
> >       > --greg.
> >       >
> >       >
> >       > >
> >       > > The normal behavior then is that
we are waking the thread up
> >       > > pretty often at the beginning
until the output
> > queue is filled
> >       > > up. By this time the input queue
usually holds a
> > lot of input
> >       > > encoded frames. After the output
queue is filled up, then we
> >       > > wake up the decoder thread only as
needed.
> >       > >
> >       > > Also, there were a few places
(like when we were paused)
> >       > > that the base video renderer
suspended the decode pacemaker.
> >       > > Since we are ALWAYS running the
decode pacemaker in
> > suspended
> >       > > mode, then there is no need to
make these calls.
> >       > >
> >       > > Files Modified
> >       > >
-----------------------------------------
> >       > >
datatype/common/vidrend/vidrend.cpp
> >       > >
datatype/common/vidrend/vidrendf.cpp
> >       > >
datatype/common/vidrend/pub/vidrend.h
> >       > >
> >       > > Branches
> >       > >
-----------------------------------------
> >       > > HEAD and hxclient_3_1_0_atlas
> >       > >
> >       > > Testing
> >       > >
-----------------------------------------
> >       > > I built all the vidrend-derived
renderers and played
> >       > > back video using the RealVideo,
Windows Media Video,
> >       > > and MPEG4 video renderers. I
tested normal playback
> >       > > and seeking using splay. I also
verified from logs
> >       > > that all frames were decoded and
bltted.
> >       > >
> >       > >
=====================================
> >       > > Eric Hyche, Technical Lead
> >       > > RealNetworks, Inc.
> >       > > ehychereal.com
> >       > >
> >       > >
> >       > >
> >       >
------------------------------------------------------------
--
> >       > ----------
> >       > >
> >       > >
_______________________________________________
> >       > > Datatype-dev mailing list
> >       > > Datatype-devhelixcommunity.org
> >       > >
> > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
> > <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>
> >       >
> >
> >
> >      
_______________________________________________
> >       Datatype-dev mailing list
> >       Datatype-devhelixcommunity.org
> >
> > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
> > <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>
> >
> >
> >



_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev

Milko's suggestions have been incorporated and
this has been checked into HEAD and 310Atlas.

=============================================
Eric Hyche (ehychereal.com)
Technical Lead
RealNetworks, Inc.  

> -----Original Message-----
> From: Milko Boic [mailto:milkoreal.com] 
> Sent: Monday, April 21, 2008 5:24 PM
> To: ehychereal.com; 'Greg Wright'
> Cc: datatype-devlists.helixcommunity.org
> Subject: RE: [datatype-dev] CR-Client: change base
video 
> renderer decoder thread polling behavior
> 
> 
>  >In OnPreSeek(), however, if we call  
> m_pVideoFormat->SuspendDecode(TRUE),
>  >then there is no need for this:
>  >
>  >     // Suspend the Decoder pump
>  >     if (m_pDecoderPump)
>  >     {
>  >         m_pDecoderPump->Suspend(TRUE);
>  >         m_pDecoderPump->Signal();
>  >         m_pDecoderPump->WaitForSuspend();
>  >     }
>  >
>  >
>  >since it has no effect at all. The call to 
> m_pVideoFormat->SuspendDecode(TRUE)
>  >sets CVideoFormat::m_bDecodeSuspended to TRUE, and
in the loop
>  >logic in CVideoFormat:ecodeFra
me(), if m_bDecodeSuspended is TRUE,
>  >then CVideoFormat:ecodeFra
me() is guaranteed to not do 
> any decoding.
> 
> It is Signal and WaitForSuspend that have the effect. 
These 
> statements 
> make sure that decoder has indeed made notice of
SuspendDecode() 
> directive.  Thus, for statements that follow
WaitForSuspend() it is 
> guaranteed that decoder will not be moving data from
input to 
> output queue 
> or operating on locally held data.  This is important
to make 
> sure the 
> decoder avoids the race condition with Reset in
statements below and 
> thus  complete flush of the data is accomplished.
> 
> Milko
> 
> 
> At 01:38 PM 4/21/2008, Eric Hyche wrote:
> 
> >Milko,
> >
> >Thanks for the review. Comments inline below...
> >
> > > -----Original Message-----
> > > From: Milko Boic [mailto:milkoreal.com]
> > > Sent: Monday, April 21, 2008 4:09 PM
> > > To: ehychereal.com; 'Greg Wright'
> > > Cc: datatype-devlists.helixcommunity.org
> > > Subject: RE: [datatype-dev] CR-Client: change
base video
> > > renderer decoder thread polling behavior
> > >
> > >
> > > Two comments:
> > >
> > > 1.) I would eliminate the check for FULL
queue on retrieval
> > > from the post-decode (output) queue and
Signal in all cases
> > > upon retrieval from the output queue.  The
reason is that
> > > current logic creates a potential deadlock
condition:
> > >           - out queue is non-full
> > >         - decoder runs and fills-up the out
queue
> > >           - new packets arrive but decoder is
not signaled
> > > since out-queue is full
> > >         - renderer retrieves frame from the
out queue
> > >         - renderer does not signal the
decoder since out
> > > queue wasn't full
> > >
> > >         The above situation will result in
newly arrived
> > > packets not getting decoded until another
packet arrives.
> > > This could be late or never.
> > >
> > >         I suggest to always signal the
decoder on retrieved
> > > frame from the out queue.  Given that input
queue is full in
> > > normal cases, this will result in useful
signalling in most cases.
> > >
> >
> >Sounds good - I will make this change.
> >
> > >
> > > 2.) When entering seek, the removed code was
making
> > > preventing the decoder from using the CPU for
pre-seek frames
> > > thus improving the responsiveness of the seek
operation.  We
> > > need to keep this functionality.  Since
decoder is now
> > > signaled causally, we cannot just rely on
suspending the
> > > thread but we must also instruct the decoder
not to decode
> > > when invoked.
> >
> >Ok, I see the logic for adding
m_pVideoFormat->SuspendDecode(TRUE)
> >in OnPreSeek() and
m_pVideoFormat->SuspendDecode(FALSE) in
> >OnPostSeek(). This will prevent us from actually
doing any
> >decoding if we get any packets we receive in
between OnPreSeek()
> >and OnPostSeek().
> >
> >In OnPreSeek(), however, if we call  
> m_pVideoFormat->SuspendDecode(TRUE),
> >then there is no need for this:
> >
> > >     // Suspend the Decoder pump
> > >     if (m_pDecoderPump)
> > >     {
> > >         m_pDecoderPump->Suspend(TRUE);
> > >         m_pDecoderPump->Signal();
> > >         m_pDecoderPump->WaitForSuspend();
> > >     }
> > >
> >
> >since it has no effect at all. The call to 
> m_pVideoFormat->SuspendDecode(TRUE)
> >sets CVideoFormat::m_bDecodeSuspended to TRUE, and
in the loop
> >logic in CVideoFormat:ecodeFra
me(), if m_bDecodeSuspended is TRUE,
> >then CVideoFormat:ecodeFra
me() is guaranteed to not do any 
> decoding.
> >
> >In OnPostSeek(), I agree we need to call 
> m_pVideoFormat->SuspendDecode(FALSE),
> >but we definitely do NOT want to do this:
> >
> > >     if (m_pDecoderPump)
> > >     {
> > >         m_pDecoderPump->Suspend(FALSE);
> > >         m_pDecoderPump->Signal();
> > >     }
> >
> >since this will take the decoder pacemaker out of
suspended mode
> >and return it to making calls every DECODE_INTEVAL
ms. We want
> >to run the decoder pacemaker in suspended mode all
> >the time.
> >
> >=============================================
> >Eric Hyche (ehychereal.com)
> >Technical Lead
> >RealNetworks, Inc.
> >
> > > Suspending and waiting for decoder pump to
suspend is still
> > > useful to sync the decoded suspension state
and thus make
> > > sure that flush of the queues is complete.
> > >
> > > STDMETHODIMP
CVideoRenderer::OnPreSeek(ULONG32 ulOldTime,
> > > ULONG32 ulNewTime)
> > > {
> > >     HXLOGL3(HXLOG_BVID,
> > > "CVideoRenderer::OnPreSeek(OldTime=%lu,
NewTime=%lu)",
> > > ulOldTime, ulNewTime);
> > >
> > > +   m_pVideoFormat->SuspendDecode(TRUE);
> > >
> > >     // Change state to stop Blts
> > >     m_pMutex->Lock();
> > >     EndBuffering();
> > >     m_PlayState = Seeking;
> > >     m_pMutex->Unlock();
> > >
> > >     // Suspend the Decoder pump
> > >     if (m_pDecoderPump)
> > >     {
> > >         m_pDecoderPump->Suspend(TRUE);
> > >         m_pDecoderPump->Signal();
> > >         m_pDecoderPump->WaitForSuspend();
> > >     }
> > >
> > >     // Wait for Blt in progress to complete
and reset
> > >     // packet queues
> > >     DisplayMutex_Lock();
> > >
> > > STDMETHODIMP
CVideoRenderer::OnPostSeek(ULONG32 ulOldTime,
> > > ULONG32 ulNewTime)
> > > {
> > >     HXLOGL3(HXLOG_BVID,
> > > "CVideoRenderer::OnPostSeek(OldTime=%lu,
NewTime=%lu)",
> > > ulOldTime, ulNewTime);
> > >
> > >     DisplayMutex_Lock();
> > >
> > > +  m_pVideoFormat->SuspendDecode(FALSE);
> > >
> > >     if (m_pDecoderPump)
> > >     {
> > >         m_pDecoderPump->Suspend(FALSE);
> > >         m_pDecoderPump->Signal();
> > >     }
> > >
> > >     if (m_bUseVideoSurface2 &&
m_pMISUSSite)
> > >     {
> > >         FlushVideoSurface2(m_pMISUSSite);
> > >     }
> > >
> > >
> > >
> > > At 01:16 PM 4/18/2008, Eric Hyche wrote:
> > >
> > >
> > >
> > >       > -----Original Message-----
> > >       > From: Greg Wright [
mailto:gwrightreal.com
> > > <mailto:gwrightreal.com> ]
> > >       > Sent: Friday, April 18, 2008 4:04
PM
> > >       > To: ehychereal.com
> > >       > Cc: datatype-devlists.helixcommunity.org
> > >       > Subject: Re: [datatype-dev]
CR-Client: change base video
> > >       > renderer decoder thread polling
behavior
> > >       >
> > >       > Eric Hyche wrote:
> > >       > > Description
> > >       > >
-----------------------------------------
> > >       > > When we decode on a separate
thread in the base
> > > video renderer,
> > >       > > we previously set up an
IHXPaceMaker and had it call us
> > >       > > back to our OnPace() method
regularly every 5ms.
> > > Most of the time,
> > >       > > of course, there are no
frames to decode and so the call
> > >       > > to OnPace() does nothing.
However, this 
> constant sleeping
> > >       > > and waking of the internal
IHXPaceMaker thread results
> > >       > > in a lot of power consumption
on mobile 
> devices, as reported
> > >       > > by Intel.
> > >       > >
> > >       > > Therefore the goal of this
change is to wake the
> > > decoder thread
> > >       > > up as few times as possible,
and only when 
> there is encoded
> > >       > > frames to decode and space in
the decoded frame
> > > output queue.
> > >       > >
> > >       > > To accomplish this, we run
the decoder pacemaker in
> > > suspended
> > >       > > mode all the time (using the
new suspend internal
> > > behavior of
> > >       > > CVideoPaceMaker). Therefore
in suspended mode, the
> > > decode thread
> > >       > > will be asleep until we
signal it. When we signal
> > > it, we will get
> > >       > > one callback to our OnPace
method and then the
> > > thread will go
> > >       > > back to sleep.
> > >       > >
> > >       > > Currently, each time we get
an OnPace call, the
> > > base video renderer
> > >       > > decodes as many frames as it
can. It will decode
> > > frames until
> > >       > > either: a) it has exhausted
the input queue of
> > > encoded frames; or
> > >       > > b) it has exhausted the space
in the decoded frame
> > > output queue.
> > >       > >
> > >       > > Therefore, we signal the
decoder thread if either one
> > >       > > of these conditions occur:
> > >       > >
> > >       > > 1) if an assembled encoded
frame was just added to
> > > the input queue
> > >       > >    AND there is currently
space in the output queue;
> > >       > > 2) if we just removed a
decoded frame from a FULL
> > > output queue.
> > >       >
> > >       > 2) ...and there are encoded frames
to decode?
> > >       >
> > >
> > >       Well, checking that would require
grabbing the 
> mutex protecting
> > >       the input queue, and I wanted to avoid
doing that for risk
> > >       of deadlock. Given that the input queue
generally remains
> > >       pretty full almost all the time,
avoiding the input queue
> > >       check only results in a few extra
checks at the very end
> > >       of playback (on the order of 10 extra
wakeups total over
> > >       the entire playback).
> > >
> > >       This change already reduces the number
of thread wakeups
> > >       from roughly 200/second (we were asking
for the thread to
> > >       call us back every 5ms) to about
23/second - about a 90%
> > >       reduction.
> > >
> > >       Eric
> > >
> > >       >
> > >       > rest looks good.
> > >       > --greg.
> > >       >
> > >       >
> > >       > >
> > >       > > The normal behavior then is
that we are waking 
> the thread up
> > >       > > pretty often at the beginning
until the output
> > > queue is filled
> > >       > > up. By this time the input
queue usually holds a
> > > lot of input
> > >       > > encoded frames. After the
output queue is 
> filled up, then we
> > >       > > wake up the decoder thread
only as needed.
> > >       > >
> > >       > > Also, there were a few places
(like when we were paused)
> > >       > > that the base video renderer
suspended the 
> decode pacemaker.
> > >       > > Since we are ALWAYS running
the decode pacemaker in
> > > suspended
> > >       > > mode, then there is no need
to make these calls.
> > >       > >
> > >       > > Files Modified
> > >       > >
-----------------------------------------
> > >       > >
datatype/common/vidrend/vidrend.cpp
> > >       > >
datatype/common/vidrend/vidrendf.cpp
> > >       > >
datatype/common/vidrend/pub/vidrend.h
> > >       > >
> > >       > > Branches
> > >       > >
-----------------------------------------
> > >       > > HEAD and
hxclient_3_1_0_atlas
> > >       > >
> > >       > > Testing
> > >       > >
-----------------------------------------
> > >       > > I built all the
vidrend-derived renderers and played
> > >       > > back video using the
RealVideo, Windows Media Video,
> > >       > > and MPEG4 video renderers. I
tested normal playback
> > >       > > and seeking using splay. I
also verified from logs
> > >       > > that all frames were decoded
and bltted.
> > >       > >
> > >       > >
=====================================
> > >       > > Eric Hyche, Technical Lead
> > >       > > RealNetworks, Inc.
> > >       > > ehychereal.com
> > >       > >
> > >       > >
> > >       > >
> > >       > 
>
------------------------------------------------------------
--
> > >       > ----------
> > >       > >
> > >       > >
_______________________________________________
> > >       > > Datatype-dev mailing list
> > >       > > Datatype-devhelixcommunity.org
> > >       > >
> > > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
> > > <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>
> > >       >
> > >
> > >
> > >      
_______________________________________________
> > >       Datatype-dev mailing list
> > >       Datatype-devhelixcommunity.org
> > >
> > > http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev
> > > <http://lists.helixcommunity.org/mailman/listinf
o/datatype-dev>
> > >
> > >
> > >
> 
> 


_______________________________________________
Datatype-dev mailing list
Datatype-devhelixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/da
tatype-dev