Sateesh: Here are my comments on the added files:
audio/tonegen: 1) It appears the source file headers are out of date. Headers can be found here: https://helix.dev.prognet.com/index.cgi/SourceFileHeaders Added C++ files should use the header in 2.1.1. Added Python files should use the header in 2.2.1. 2) In audio/tonegen, there doesn't seem to be any need for a sub-Umakefil (Umakefil referencing libumakefil, which has the real build instructions). Just put the code which is currently in libumakefil into Umakefil and eliminate the MultiTargetMake() in Umakefil. 3) Don't check in tonerenderer.def, since .def files are generated by the build system. 4) tonegen.cpp needs a license header. 5) tongen.h needs a license header. datatype/tone/fileformat: 1) License file headers are out of date - see above 2) MLOG_xxx has been deprecated as a logging system. You should use HXLOG, as documented here: https://common.helixcommunity.org/2005/devdocs/UnifiedLogging Therefore, toneffmlog.h should not be needed. 3) Code lines that have been commented out should be removed, unless they are needed to explain some behavior or we think we might eventually switch over to using the commented out code. 4) I see that Seek() returns HXR_UNEXPECTED, but I saw in your status report that you are still working on getting seeking working. So I assume that the Seek() method will be changed in a separate CR. Is that correct? 5) This code block in toneff.cpp: // Call back to the response IHXFileMimeMapper* pMapper = NULL; m_pFileObject->QueryInterface(IID_IHXFileMimeMapper, (void**) &pMapper); if (pMapper) { // Get the URL const char* pszURL = NULL; m_pRequest->GetURL(pszURL); if (pszURL) { // Get our own response interface IHXFileMimeMapperResponse* pResponse = NULL; QueryInterface(IID_IHXFileMimeMapperResponse, (void**) &pResponse); if (pResponse) { // Call FindMimeType - look in MimeTypeFound for // the response pMapper->FindMimeType(pszURL, pResponse); } HX_RELEASE(pResponse); } } HX_RELEASE(pMapper); doesn't make any sense. CTONEFileFormat doesn't even support IHXFileMimeMapperResponse so FindMimeType will never be called. Why do you need to know the mime type of the file? 6) In SeekDone(), you have this: retVal = m_pFileObject->Read(8);//TONE_HEADER_READ_SIZE If TONE_HEADER_READ_SIZE is 8, then just use it in the Read() call: retVal = m_pFileObject->Read(TONE_HEADER_READ_SIZE); 7) These two comments don't make sense beside each other: // Now read TONE_HEADER_READ_SIZE bytes //No Header Information available to read retVal = m_pFileObject->Read(8);//TONE_HEADER_READ_SIZE I suspect one of them is a left-over from the old code that you used as a template for this new code. Please remove the inapplicable comment. 8) In GetFileHeader, you first Seek() to the beginning of the file, then in SeekDone(), you read 8 bytes of the file, and then in ReadDone(), you don't do anything with those 8 bytes you just read. Then in GetStreamHeader(), you Seek() back to the beginning of the file again. So it looks to me as if the seek and read started by GetFileHeader are completely unnecessary. If so, remove them since they are just adding unnecessary code size and complexity. 9) Remove commented-out code in GetStreamHeader() 10) I don't understand this logic: // Create a raw file packet IHXPacket* pPacket = NULL; retVal = MakeRawFilePacket(pBuffer, m_ulNextTimeStamp, pPacket); if (SUCCEEDED(retVal)) { // Process this input packet into output queue packets. // We could have done this either in SetPacket() or GetPacket(), // but we choose to do it here. We don't force flushing yet, // since we may get more data. Once we get a call to flush, // then we process all the input, regardless of min size. pHdr->GetPropertyULONG32("MinPacketSize", m_ulMinPacketSize); retVal = ProcessInputPacket(pPacket, m_bFlush, m_ulMinPacketSize, m_ulPacketBytesConsumed, m_ulDurationConsumed); if (SUCCEEDED(retVal)) { // Update the next file offset m_ulNextFileOffset += GetPacketBytesConsumed(); // Update the next time stamp m_ulNextTimeStamp += GetDurationConsumed(); // Set the state m_eState = StateReady; // Send the stream header retVal = m_pFormatResponse->StreamHeaderReady(HXR_OK, pHdr); } } HX_RELEASE(pPacket); You are taking pBuffer and putting it into a packet in MakeRawFilePacket(), then turning right around in ProcessInputPacket() and pulling the buffer back out and making a new different packet out of it. Why? It seems like the first creation of the packet is not necessary. 11) Remove commented-out code in GetPacket() 12) This code block in ReadDone() will never get executed: if (m_bScanForFrameBegin) { ... } Why is it there? It looks to me like it's left-overs from the code you started with... 13) Same comment as #10 above in this code block in ReadDone() else if (m_eState == StateGetPacketReadDonePending) { ... } It appears there is an unnecessary step of creating a packet. 14) FindAllTONEFramesLength() does not appear to be called at all. 15) The file win32.pcf should actually be called toneffdll_win32.pcf. If it is called win32.pcf then it is applied to both tonefflib and toneffdll, and it only needs to be applied to toneff. 16) In toneff.h, there are these definitions: const char VERSION = (UINT8)(-2); const char TEMPO = (UINT8)(-3); This is needlessly confusing. You are taking a negative number, casting it to unsigned number, and then assigning it to a char. And then in the code you are comparing that to a BYTE. Also, these are declared as global variables and there is no need for them to be. They can just be preprocessor definitions. Why not just make the definitions clear as in: #define HXTONE_SILENCE 0xFF #define HXTONE_C4 0x3C ... 17) In toneff.h, these are public methods: UINT32 GetPacketBytesConsumed() { return m_ulPacketBytesConsumed; } UINT32 GetDurationConsumed() { return m_ulDurationConsumed; } and they should be private. datatype/tone/renderer: 1) In libumakefil, you have: project.AddModuleLibraries("audio/tonegen[tonegenlib]") There is no need in a LibraryTarget to have an AddModuleLibraries, since libs don't link with anything. Instead, just add this line to your AddModuleIncludes: "audio/tongen/pub", Also, is adding "audio/tonegen" to the AddModuleIncludes really needed? 2) In tonefmt.cpp in CreateAssembledPacket(), it looks like it only creates m_pMediapkt for the first IHXPacket passed in and then just keeps returning m_pMediapkt. Is this really what you wanted? So any packets after the first one are just thrown away? 3) In CToneAudioFormat::DecodeAudioData(), it appears that you create a temporary IHXBuffer in m_pInputBuffer and then pass it into ParseAudioData(). However, you already have an IHXBuffer() in the CMediaPacket. Therefore, it appears that this is a needless memcpy. Why not just use the IHXBuffer that already exists in the CMediaPacket? Also, on the output, it appears that you are copying the output PCM again into a new IHXBuffer output buffer. What is the reason for this extra copy on the output? Now the changed files: > > -const char* DataFileSystem::zm_pProtocol = "data"; > > - > > +//#ifdef HELIX_FEATURE_TONE_GENERATOR > > +const char* DataFileSystem::zm_pProtocol = "data|tone"; > > +//#else > > +//const char* DataFileSystem::zm_pProtocol = "data|tone"; > > +//#endif > > int g_nRefCount_datafsys = 0; > > Shouldn't this be: -const char* DataFileSystem::zm_pProtocol = "data"; - +#ifdef HELIX_FEATURE_TONE_GENERATOR +const char* DataFileSystem::zm_pProtocol = "data|tone"; +#else +const char* DataFileSystem::zm_pProtocol = "data"; +#endif In other words, if HELIX_FEATURE_TONE_GENERATOR is not defined, then datafsys only claims the "data://" scheme. > > +#ifdef HELIX_FEATURE_TONE_GENERATOR > > +const char VERSION = (UINT8)(-2); > > +const char TEMPO = (UINT8)(-3); > > +const char RESOLUTION = (UINT8)(-4); > > +const char BLOCK_START = (UINT8)(-5); > > +const char BLOCK_END = (UINT8)(-6); > > +const char PLAY_BLOCK = (UINT8)(-7); > > +const char SET_VOLUME = (UINT8)(-8); > > +const char C4 = (UINT8)(60); > > +const char REPEAT = (UINT8)(-9); > > +const char SILENCE = (UINT8)(-1); > > +#endif > > I saw these definitions somewhere else - I think in the file format. They should NOT be duplicated, but rather put into a common header file. > > + IHXBuffer* pData, IHXRequest* pRequest); > > + > > +#define HELIX_FEATURE_TONE_GENERATOR 1 > > +#ifdef HELIX_FEATURE_TONE_GENERATOR > > + STDMETHOD(SetSequence)(UINT32 ulNote, UINT32 ulToneDuration, HELIX_FEATURE_TONE_GENERATOR should not be hard-coded in the code anywhere. Instead, it should be placed into the appropriate profile. That's all. Eric ============================================== Eric Hyche ([EMAIL PROTECTED]) Technical Lead Embedded Player and Technologies RealNetworks, Inc. _______________________________________________ Audio-dev mailing list [email protected] http://lists.helixcommunity.org/mailman/listinfo/audio-dev
