RE: [Re-Send] Add power save mode support for offload music playback (Framework part only)

Looks good.

fxd

On Thu, 2009-09-17 at 18:48 +0800, Zhao, Halley wrote:
> Thanks Sheldon.
> Here is my follow up
> 
> 1. Move pIPowerSave to be a class member variable m_ pIPowerSave, and release it during class destroy. (CHXClientEngine.cpp and hxaudply.cpp)
> 2,3,4,5 followed your suggestion.
> 6. use base class HXSource for cast, and add 	HXNetSource with:        virtual HXBOOL UsesNetworkAccess(){return TRUE;} ;
> 
> 
> -----Original Message-----
> From: Sheldon fu [mailto:[email protected]] 
> Sent: Tuesday, September 01, 2009 4:42 AM
> To: Zhao, Halley
> Cc: '[email protected]'
> Subject: Re: [Helix-client-dev] [Re-Send] Add power save mode support for offload music playback (Framework part only)
> 
> 1. pIPowerSave needs to be released in the new methods added to
> CHXClientEngine.cpp.
> 
> 2. Can not return HXR_FAIL from methods with HXBOOL (or anything other
> than HX_RESULT) return type, in file CHXClientEngine.cpp.
> 
> 3. STDMETHODIMP_ shouldn't be used in the header file CHXClientEngine.h.
> Should just use STDMETHOD or STDMETHOD_
> 
> 4. In hxcore.h, "STDMETHOD_(HX_RESULT,xxx)" should be just
> "STDMETHOD(xxx)".
> 
> 5. In hxcleng.cpp
> 
> +        LISTPOSITION lPosition = m_PlayerList.GetHeadPosition();
> +        if (lPosition != NULL)
> +        {
> +            HXPlayer* pPlayer = (HXPlayer*)
> m_PlayerList.GetAt(lPosition);
> +            if (pPlayer != NULL)
> +                pPlayer->AdjustWakeUpInterval(m_ulWakeUpInterval);
> 
> can be simplified by using m_PlayerList.GetHead or
> m_PlayerList.GetTail().
> 
> 6. Helix coding convention normally suggests putting body of conditional
> statements in '{}' pair, at least in the client core source files. e.g, 
> 
> +    if (theErr == HXR_OK)
> +        m_bInPowerSave = TRUE;
> +    else
> +        EndPowerSave();
> 
> would be
> 
> +    if (theErr == HXR_OK)
> +    {
> +        m_bInPowerSave = TRUE;
> +    }
> +    else
> +    {
> +        EndPowerSave();
> +    }
> 
> 7. in hxplay.cpp
> 
> +            if (((HXFileSource*)pSource)->UsesNetworkAccess())
> 
> is dangerous. You may be casting a HXNetSource to HXFileSource.
> 
> The overall logic looks ok to me.
> 
> I assume this is un-tested code since we don't have any audio renderer
> that supports IHXPowerSave interface and behavior yet, right?
> 
> fxd
> 
> On Fri, 2009-08-28 at 10:21 +0800, Zhao, Halley wrote:
> > Send it again since there is no response yet.
> > 
> >  
> > 
> >                                    
> > ______________________________________________________________________
> > From:[email protected]
> > [mailto:[email protected]] On Behalf Of
> > Zhao, Halley
> > Sent: 2009年8月20日15:09
> > To: '[email protected]'
> > Subject: [Helix-client-dev] Add power save mode support for offload
> > music playback (Framework part only)
> > 
> > 
> >  
> > 
> > Synopsis: 
> > 
> > Some audio dsp could playback(decode and render) long period of audio
> > data without interaction with Helix core.
> > 
> > we need reduce wakeup from Helix audio service to achieve power
> > saving.
> > 
> > This is achieved by:
> > 
> >     audio codec/render doesn't register audio stream to audio service
> > idle.
> > 
> >     audio codec/render register as a IHXClockSource to drive the
> > timeline
> > 
> >                                
> > 
> > Overview: 
> > 
> >     Details see attached design-note.txt
> > 
> >          
> > 
> > Files Added:
> > 
> >        No file added
> > 
> > Files Modified:
> > 
> > |-- client
> > 
> > |   |-- audiosvc
> > 
> > |   |   |-- hxaudply.cpp
> > 
> > |   |   |-- hxaudses.cpp
> > 
> > |   |   `-- pub
> > 
> > |   |       |-- hxaudply.h
> > 
> > |   |       `-- hxaudses.h
> > 
> > |   `-- core
> > 
> > |       |-- hxbsrc.h
> > 
> > |       |-- hxcleng.cpp
> > 
> > |       |-- hxflsrc.cpp
> > 
> > |       |-- hxflsrc.h
> > 
> > |       |-- hxplay.cpp
> > 
> > |       `-- pub
> > 
> > |           |-- hxcleng.h
> > 
> > |           `-- hxplay.h
> > 
> > |-- common
> > 
> > |   `-- include
> > 
> > |       |-- hxausvc.h
> > 
> > |       |-- hxcore.h
> > 
> > |       `-- hxevent.h
> > 
> > |-- list.txt
> > 
> > `-- player
> > 
> >     `-- hxclientkit
> > 
> >         |-- pub
> > 
> >         |   `-- HXClientCFuncs.h
> > 
> >         `-- src
> > 
> >             |-- CHXClientEngine.cpp
> > 
> >             |-- CHXClientEngine.h
> > 
> >             `-- HXClientCFuncs.cpp
> > 
> >  
> > 
> > Image Size and Heap Use impact (Client -Only):
> > 
> >     little
> > 
> > Platforms and Profiles Affected:
> > 
> >     platform:   linux-2.2-libc6-gcc32-i586
> > 
> >     profile:    helix-client-all-defines
> > 
> >  
> > 
> > Distribution Libraries Affected:
> > 
> >     <hxmedpcleng.so>
> > 
> > Distribution library impact and planned action:
> > 
> >     <None>
> > 
> > Platforms and Profiles Build Verified:
> > 
> >     Set BIF branch  -> realplay_gtk_atlas_restricted
> > 
> >     Set Target(s)   -> player_all
> > 
> >     Set Profile     -> helix-client-moblin
> > 
> >     System ID       -> linux-2.2-libc6-gcc32-i586
> > 
> > Branch: 
> > 
> >     HEAD
> > 
> > Copyright assignment: <MUST be one of the following statements >
> > 
> >    2.      Intel has signed and delivered a Joint Copyright Assignment
> > 
> >            to RealNetworks, and received acknowledgment that the 
> > 
> >            agreement was received. 
> > 
> > Files Attached:
> > 
> > Design-note.txt, client-audiosvc.diff.txt, client-core.diff.txt,
> > common-include.diff.txt, player-hxclientkit.diff.txt
> > 
> >  
> > 
> >  
> > 
> > ZHAO, Halley (Aihua)
> > 
> > Email: [email protected]
> > 
> > Tel: +86(21)61166476
> > 
> > iNet: 8821-6476
> > 
> > SSG/OTC/Moblin 3W038 Pole: F4
> > 
> > 
> >  
> > 
> > 
> > _______________________________________________
> > Helix-client-dev mailing list
> > [email protected]
> > http://lists.helixcommunity.org/mailman/listinfo/helix-client-dev
>