Thanks Sheldon! Checked in to 210CayS, Head and 420Brizo  

/Gang

-----Original Message-----
From: ext Sheldon Fu [mailto:sfu at real.com] 
Sent: Thursday, March 04, 2010 5:10 PM
To: Jia Gang (Nokia-D/Dallas)
Cc: datatype-dev at helixcommunity.org; nokia-private-dev at helixcommunity.org
Subject: Re: [Nokia-private-dev] CR:Case ou1cimx1#259923 Helix Cannot play MPEG4 avi with 5.1ch AC3

Looks good.

fxd

Gang.Jia at nokia.com wrote:
> Hi Sheldon
>
> I have taken the comments 1,2,3. For comment 4, I have removed the extra copy. Now there is only one copy in the fragmented case, for none fragmented case there is no copy in DDDepack. Since the clip with fragmented frame is very rare I think this change should have little impact on the performance.
>
> Thanks,
> Gang
>
>
> -----Original Message-----
> From: ext Sheldon Fu [mailto:sfu at real.com]
> Sent: Wednesday, March 03, 2010 7:41 PM
> To: Jia Gang (Nokia-D/Dallas)
> Cc: datatype-dev at helixcommunity.org; 
> nokia-private-dev at helixcommunity.org
> Subject: Re: [Nokia-private-dev] CR:Case ou1cimx1#259923 Helix Cannot 
> play MPEG4 avi with 5.1ch AC3
>
> Change in mp4pyld looks good. For dd-depack.cpp:
>
> 1. Init function always return TRUE even if there is out of memory error(s). If we don't think OOM could happen, then those detection logic should be taken out, otherwise correct error coder returned.
>
> 2. dtor's "if (xxx) delete(xxx); xxx = 0;" could be replaced by "HX_VECTOR_DELETE(xxx)"
>
> 3. In FindSyncWord, the outer 'if' doesn't seem to be needed if you just make the for loop start at i=0, instead of i=1.
>
> 4. The depack logic in OnPacket and ProcessingStagingBuffer is quite inefficient with extra memcpy ops that can be avoided, especially for clips that don't have fragmented packets (or have only a small amount of fragmented  packets). For any incoming packet, if it doesn't start with partial frame data (has sync word at beginning) then the data up to the last whole frame in the packet buffer  can be returned to the caller directly without any memcpy. Only for incoming packet that starts at partial frame offset that memcpy is needed and even for that case only one memcpy is needed for any given byte of the incoming data. Having a staging buffer and a final buffer to always do two memcpy ops for each byte is not necessary.
>
> Since this has some performance impact (memcpy stuff), It might be a good idea to wrap the calling of depacktizer from mp4pyld in a HELIX_FEATURE define.
>
> fxd
>
> Gang.Jia at nokia.com wrote:
>   
>> Any comment on this one?
>>  
>> Thanks
>> Gang
>>
>> ---------------------------------------------------------------------
>> -
>> --
>> *From:* nokia-private-dev-bounces at helixcommunity.org
>> [mailto:nokia-private-dev-bounces at helixcommunity.org] *On Behalf Of 
>> *Jia Gang (Nokia-D/Dallas)
>> *Sent:* Monday, March 01, 2010 3:22 PM
>> *To:* datatype-dev at helixcommunity.org
>> *Cc:* nokia-private-dev at helixcommunity.org
>> *Subject:* [Nokia-private-dev] CR:Case ou1cimx1#259923 Helix Cannot 
>> play MPEG4 avi with 5.1ch AC3
>>
>> "Nokia submits this code under the terms of a commercial contribution 
>> agreement with RealNetworks, and I am authorized to contribute this 
>> code under said agreement."
>>  
>> Modified by:  Gang.Jia at nokia.com
>>  
>> Reviewed by:
>>  
>> Date: 2/26/2010
>>  
>> Project: SymbianMmf_wm
>>  
>> ErrorId:  ESLM-829J6K
>>  
>> Synopsis: CR: Helix Cannot play MPEG4 avi with 5.1ch AC3
>>  
>> The current implementation of Dolby Digital doesn't consider the case 
>> that the audio frame could be fragmented during the encoding. The 
>> clip used in this error case is encoded with audio frame fragmented. 
>> To handle this we added de-packetizer for Dolby Digital/Dolby Digital 
>> plus to reassemble the fragmented frames. The de-packetizer tries to 
>> find the sync word of each frame. Based on the sync word it can get 
>> the frame length and then do the frame reassemble.
>>  
>> Root Cause of the problem:  Implementation.
>>  
>> Files Added:
>> /datatype/mp4/payload/dd_depack.cpp
>> /datatype/mp4/payload/pub/dd_depack.h
>>  
>> Files Modified:
>> /datatype/mp4/payload/umakefil
>> /datatype/mp4/payload/mp4apyld.cpp
>> /datatype/mp4/payload/pub/mp4apyld.h
>>  
>> Image Size and Heap Use impact:
>> mp4pyldlib.lib size increased 2kB
>> Heap size increased based on encoded AC3/EAC3 frame sizes, <20KB in 
>> normal cases.
>>  
>>  
>> Module Release testing (STIF) :  Passed selected test cases.
>>  
>> Test case(s) Added  : No.
>>  
>> Memory leak check performed : Yes. No new leaks introduced
>>  
>> Platforms and Profiles Build Verified: 
>> helix-client-s60-52-mmf-mdf-dsp
>>  
>> Platforms and Profiles Functionality verified: armv5
>>  
>> Branch: 210CayS, Head, 420Brizo
>>  
>>  
>>  
>>     
>
>