[Helix-client-dev] CR: Fix for stopping the playback of mp3 files when no more packets are arriving
Milko Boic milko at real.com
Looks good.
Per Helix Coding Spec, we always use {} for conditional statements:
if
{
<body>
}
The editor settings are indent = 4, tab = 8 and "fill tabs with space" .
Thanks,
Milko
At 12:11 PM 4/2/2008, Jyotsana Rathore wrote:
>Hi Milko,
>
>Please find the new diff file attached.
>Thanks for explaining your comments, its helping me understand the code
>better.
>
>-Jyotsana
>
>----------
>From: Milko Boic [mailto:milko at real.com]
>Sent: Tuesday, April 01, 2008 9:38 PM
>To: Jyotsana Rathore; helix-client-dev at helixcommunity.org
>Subject: RE: [Helix-client-dev] CR: Fix for stopping the playback of mp3
>files when no more packets are arriving
>
>
>Jyotsana,
>
>-> UINT32 variables should be prefixed with ul: ulFileDuration
> mp3 file format code is old and serves as bad example.
>
>-> The duration you need to compare against is stream duration. Thus it
>is better to name the variable ulStreamDuration. For example, we could be
>playing an .flv file and audio part would be played via mp3 renderer.
>
>-> In order to pay attention to other components possibly modifying the
>duration, ulStreamDuration should be first attempted to be retrieved from
>the layout stream properties (pProps). If this retrieval fails, then
>attempt retrieval from the stream header (m_pHeader). If this retrieval
>fails as well (FAILED(m_pHeader->GetPropertyULONG32("Duration",
>ulStreamDuration)), set the duration.
>
>Thanks,
>Milko
>
>At 05:20 PM 4/1/2008, Jyotsana Rathore wrote:
>
>Please find the new diff file attached.
>
>Thanks,
>Jyotsana
>
>
>----------
>From: Milko Boic [ mailto:milko at real.com]
>Sent: Tuesday, April 01, 2008 4:10 PM
>To: Jyotsana Rathore; helix-client-dev at helixcommunity.org
>Subject: Re: [Helix-client-dev] CR: Fix for stopping the playback of mp3
>files when no more packets are arriving
>
>
>Few comments:
>
>-> We should change the duration only if existing duration is > new duration.
>-> This is log L3 rather than L4 since it is not a high frequency call.
>-> Are \t\t\t in the log statement consistent with other usage in this
>file? If not, we should not use it.
>
>Thanks,
>Milko
>
>At 03:38 PM 4/1/2008, Jyotsana Rathore wrote:
>
>Modified by: jrathore at real.com
>Date: 04:01:08
>Project: Helix DNA Client
>
>Synopsis: Fix for stopping the playback of mp3 files when no more packets
>are arriving.
>
>Overview: The player was playing nothing even when the packets had stopped
>arriving, up until the duration of the file.
>The fix is made in mp3 renderer. After the OnEndOfPackets is called on the
>renderer it computes a new duration and sets it.
>
>Files Added:
>
>Files Modified:
>mp3rend.cpp (datatype/mp3/renderer/mp3rend.cpp)
>pktparse.h (datatype/mp3/payload/pub/pktparse.h)
>
>Image Size and Heap Use impact (Client -Only):
>
>Platforms and Profiles Affected:
>Platform: win32-i386-vc7
>Profile: helix-client-all-defines
>
>Distribution Libraries Affected:
>
>Distribution library impact and planned action:
>
>Platforms and Profiles Build Verified:
>Platform: win32-i386-vc7
>Profile: helix-client-all-defines
>target(s): splay
>
>Platforms and Profiles Functionality verified:
>Platform: win32-i386-vc7
>Profile: helix-client-all-defines
>target(s): splay
>
>Branch: hxclient_3_1_0_atlas
>
>Copyright assignment: I am a RealNetworks employee.
>
>Files Attached: mp3renderer.diff
>
>-Jyotsana
>
>
>
>
>_______________________________________________
>Helix-client-dev mailing list
>Helix-client-dev at helixcommunity.org
>http://lists.helixcommunity.org/mailman/listinfo/helix-client-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.helixcommunity.org/pipermail/helix-client-dev/attachments/20080402/31a46ef7/attachment.html