My only comment: + UINT32 m_nAdvancedEncodeOpt2;
Let's follow coding guidelines and call this "m_ulAdvancedEncodeOpt2". Rest looks good. ======================================= Eric Hyche ([EMAIL PROTECTED]) Principal Engineer RealNetworks, Inc. >-----Original Message----- >From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] >Sent: Friday, September 12, 2008 12:00 PM >To: [EMAIL PROTECTED]; [EMAIL PROTECTED]; [email protected] >Cc: [EMAIL PROTECTED] >Subject: RE: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio in Helix > >Hi Eric, > >Can you please provide your comments on the latest changes so that I can >check in by today. > >thnx & regds >AD > >-----Original Message----- >From: Dhamija Anuj (EXT-InfoVisionConsultants-MSW/Dallas) >Sent: Thursday, September 11, 2008 2:31 PM >To: [EMAIL PROTECTED]; [EMAIL PROTECTED]; >[email protected] >Cc: [EMAIL PROTECTED] >Subject: RE: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio >in Helix > >Hi Eric, > >I meant channel mask is a new field in HXAudioConfiguratorWMA. > >I fixed the parse_audio.cpp channel mask bug as I am not getting correct >number of channels when filtering clips with more than 2 channels. > >Also I figured out that CWMAudioDecoder won't be the right place to put >check for number of channels. So I moved this logic to >HXAudioConfiguratorWMA::ValidateDecoderInfo as it is more decoder >specific class. Since there is no context available in >HXAudioConfiguratorWMA I read in the max number of supported channels in >HXSymbianWMASwAudioDecoder::Open() call and pass in to >HXAudioConfiguratorWMA using new API for setting max supported shannels. >This check is now removed from CWMAudioDecoder::Init(). No changes are >now made to WMADecoder.cpp > >I have removed m_nBitPerSample and m_nBlockAlign from >HXWMATypeSpecificData as they are not required. Also corrected the >naming convention for numWMAChannels [now in wmasymbianswdecoder.cpp]. > >Let me know your comments about these new changes. > >thnx & regds >AD > > > > >-----Original Message----- >From: ext Eric Hyche [mailto:[EMAIL PROTECTED] >Sent: Thursday, September 11, 2008 9:36 AM >To: Dhamija Anuj (EXT-InfoVisionConsultants-MSW/Dallas); >[EMAIL PROTECTED]; [email protected] >Cc: [EMAIL PROTECTED] >Subject: RE: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio >in Helix > >>Regarding m_usChannelMask in parse_audio.cpp, this mask is new field >>introduced with WMA10 and is not available in older versions. So I am >>not preparing a mask for WMA10 (see enclosed diff files) as it already >>has a channel mask present in the stream. Rather I have to avoid >>calling >>CWMAudioDecoder::GetNumChannels() for m_usChannels field for WMA10 [See > >>changes in method HXAudioConfiguratorWMA::ValidateDecoderConfig]. > >HXWMATypeSpecificData::m_ulChannelMask has always been there. The issue >was that I had intended to set m_ulChannelMask for TSD's that didn't >have it, but had a typo and actually wound up overwriting m_usChannels. >I'll fix that bug in parse_audio.cpp myself, after you check in your >changes. > >By the way, how does the new m_nBitPerSample differ from the existing >m_usBitsPerSample? >And how does the new m_nBlockAlign differ from the existing >m_usBlockAlign? > >One further comment using your new diffs: > > + UINT8 numWMAChannels = 0; > >Any reason why this is a UINT8 instead of a UINT16? Since we are going >to compare it against a UINT16, we might as well make it a UINT16, so we >can avoid the compiler warnings on Win32 and Linux. Also, we should use >the naming convention and call this "usWMAChannels". And I guess if we >use a UINT16, then we'll need to use ReadPrefUINT16. > >Rest looks good. > >Eric > >======================================= >Eric Hyche ([EMAIL PROTECTED]) >Principal Engineer >RealNetworks, Inc. > > >>-----Original Message----- >>From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] >>Sent: Wednesday, September 10, 2008 11:59 AM >>To: [EMAIL PROTECTED]; [EMAIL PROTECTED]; >>[email protected] >>Cc: [EMAIL PROTECTED] >>Subject: RE: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio >>in Helix >> >>Hi Eric, >> >>I erroneously sent the older diff files. Please find the latest diff >>files enclosed which take care of your comments. >>Regarding m_usChannelMask in parse_audio.cpp, this mask is new field >>introduced with WMA10 and is not available in older versions. So I am >>not preparing a mask for WMA10 (see enclosed diff files) as it already >>has a channel mask present in the stream. Rather I have to avoid >>calling >>CWMAudioDecoder::GetNumChannels() for m_usChannels field for WMA10 [See > >>changes in method HXAudioConfiguratorWMA::ValidateDecoderConfig]. >>There are other changes for updating config file to indicate support >>for WMA10 when WMA clip is played the very first time so that multiple >configurations are not required subsequently. >> >>Thnx & regds >>AD >> >>-----Original Message----- >>From: ext Eric Hyche [mailto:[EMAIL PROTECTED] >>Sent: Wednesday, September 10, 2008 8:40 AM >>To: Dhamija Anuj (EXT-InfoVisionConsultants-MSW/Dallas); >>[EMAIL PROTECTED]; [email protected] >>Cc: [EMAIL PROTECTED] >>Subject: RE: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio >>in Helix >> >>Anuj, >> >>Here are my comments: >> >>1) parse_audio.cpp >> >>+ case 1: >>+ pTSD->m_WaveFormatEx.m_usChannels = >>HXWM_SPEAKER_FRONT_CENTER; >>+ break; >>+ case 2: >>+ pTSD->m_WaveFormatEx.m_usChannels = >>HXWM_SPEAKER_FRONT_LEFT | HXWM_SPEAKER_FRONT_RIGHT; >>+ break; >> >>It looks like we should be setting pTSD->m_ulChannelMask instead of >>replacing the value of pTSD- >>>m_WaveFormatEx.m_usChannels. However, it looks like all the code in >>>parse_audio.cpp has this same >>problem. I wondered why this didn't cause a playback problem, but >>apparently it doesn't because in >>CWMAudioDecoder::GetNumChannels() we first get the number of channels >>from m_AudioTSD.m_WaveFormatEx.m_usChannels >>(which would be wrong) and then we override it with the value from the >>IHXAudioDecoder, which would be correct. So this is masking this >obvious bug in parse_audio.cpp. >> >>So I think all of the places in parse_audio.cpp which set >>pTSD->m_WaveFormatEx.m_usChannels to a channel mask should be >>changed to set the channel mask to pTSD->m_ulChannelMask instead. >> >>2) wmadecoder.cpp >> >>- Instead of having to QI for IHXPreferences, you should just use >> the alternate version of ReadPrefUINT8 which takes an IUnknown >> and does the QI for you. >> >>- Let's call the pref name "MaxSupportedWMAChannels" instead of >"MAXSupportWMAChannels" >> >> >>The rest looks good. >> >>Eric >> >>======================================= >>Eric Hyche ([EMAIL PROTECTED]) >>Principal Engineer >>RealNetworks, Inc. >> >> >>>-----Original Message----- >>>From: [EMAIL PROTECTED] >>>[mailto:[EMAIL PROTECTED] On Behalf Of >>>[EMAIL PROTECTED] >>>Sent: Tuesday, September 09, 2008 5:07 PM >>>To: [EMAIL PROTECTED]; [email protected] >>>Cc: [EMAIL PROTECTED] >>>Subject: [datatype-dev] Req ID: 106-1630 - Support WMA10 Pro Audio in >>>Helix >>> >>>"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: [EMAIL PROTECTED] >>> >>>Reviewed by: >>> >>>Req ID: 106-1630 >>> >>>Date: 09/09/2008 >>> >>>Project: Support WMA10 Pro Audio in Helix >>> >>>Synopsis: Integrate WMA10 pro decoder inside Helix while maintaining >>>backward compatible with old >>>WMA9 decoder >>> >>>Overview: >>>WMA10 pro is a super set of WMA9 decoder which is optimized for ARM >>>9t/9E and ARM11 processors. It is not backward compatible as extra >>>parameters are required to configure the decoder and different payload > >>>format is used. Same FourCC code is used to initialize the decoder. So > >>>Helix needs to figure out the available codec at runtime and support >>>both cases (old codec and new codec) accordingly >>> >>> >>>Fix: >>>A) wmasymbianswdecoder.cpp >>>i) Method ConfigDecoder introduced to handle configuration of decoder >>>based on codec available (WMA9 or WMA10) and media type. Update config >>file with availablity status of WMA10 support for next time. >>> >>>ii) Method InitWMA9Header and InitWMA10Header take care of prepending >>>header to frame. InitWMA9Header prepends same header as used by old >>>decoder. InitWMA10Header prepends header as required by new WMA10 pro >>decoder. >>> >>>B) wmaaudioconfigs.cpp >>>i) Introduce new methods to set and get format tag. These are used by >>>ConfigDecoder in wmasymbianswdecoder.cpp when configuring decoder. >>> >>>ii) Add new variables to class HXAudioConfiguratorWMA to support >>>WMA10Pro. Append these variables in config array in ConfigureDecoder >>call. >>> >>>C) wmadecoder.cpp >>>i) Read Max number of supported channels from configuration file and >>>if >> >>>number of audio channels in Audio stream are more than the config >>>parameter then reject audio. [Default is 2] >>> >>>ii) Reject 24 bit sample clips. [For now only 16-bit clips are >>>supported] >>> >>>D) parse_audio.cpp >>>i) Introduce new parameters for WMA10 to structure >>>HXWMATypeSpecificData >>>ii) Unpack new parameters for WMA10 from payload when parsing Audio >>>Data >>> >>>E) R1_Mobile_4_0_Factory.cfg >>>i) New config parameter for max supported channels for a WMA clip. >>>[MAXSupportedWMAChannels] >>> >>>Files modified & changes: >>>\datatype\mdf\audio\arm\wma\platform\symbian\wmaaudioconfigs.cpp >>>\datatype\mdf\audio\arm\wma\platform\symbian\wmasymbianswdecoder.cpp >>>\datatype\mdf\audio\arm\wma\pub\platform\symbian\wmaaudioconfigs.h >>>\datatype\mdf\audio\arm\wma\pub\platform\symbian\wmasymbianswdecoder.h >>>\datatype\wm\audio\renderer\wmadecoder.cpp >>>\datatype\wm\common\pub\parse_audio.h >>>\datatype\wm\common\parse_audio.cpp >>>\clientapps\symbiancommon\config\R1_Mobile_4_0_Factory.cfg >>> >>>Image Size and Heap Use impact: None >>> >>>Module Release testing (STIF, Audio) : Passed >>> >>>Test case(s) Added : None >>> >>>Memory leak check performed : Passed, No leaks found >>> >>>Platforms and Profiles Build Verified: helix-client-s60-32-mmf-mdf-arm >>> >>>Platforms and Profiles Functionality verified: armv5 >>> >>>Branch: Head, 210CayS >>> >>>=========================== >>>DIFF enclosed as text files >>>=========================== >>> >>>thnx & regds >>>AD >>><<Diff.zip>> >>> >>> >>> >> _______________________________________________ Audio-dev mailing list [email protected] http://lists.helixcommunity.org/mailman/listinfo/audio-dev
