[Helix-client-dev] Re: [Android-port-dev] CR: extend CHXFileRecognizer to cover more mine-types
Sheldon Fu sfu at real.comJust because it doesn't break anything, YET, doesn't mean it's correct. E.g, assuming there is a promising new file format called 'mp5', that we decide to have support for in the future, so we write a new file format plugin for it. And assume that it is similar to mp4 enough so your mp4 detection code will be fooled to think it is mp4. Since 'mp5' is not on the hard-coded list you have, your code will fail it, though we do have a file format for it. While coming back to add 'mp5' to the list will fix the issue, nobody is going to remember to do that. I hope you can see the problem with a hard-coded/arbitrary list of extensions. fxd Qiang Luo wrote: > At 09:33 AM 2/19/2009, Sheldon Fu wrote: >> 1. It's not about what you do with file with extension not in the >> list. It's about why you skip the file that has extension on the >> list. Taking '.bin' in your example, how do you know that there is >> not a file format plugin out there that claims 'bin' already or there >> won't be one in the future? Another way to think about this is to >> answer the question "how did you come up with the list?" > > I do not see any logical problem in my design. If you disagree, you > need to construct an example to show that a track would play fine with > the original code but would fail with my changes. As I explained in > my last response, having the extension list gives us the ability to > check with recognizer and play contents even if the user renames the > content file extension. Any issue followed by having this feature is > then purely performance tread off. > > Qiang > >> 2. This is another reason why a detection logic in TLC may be more >> appropriate. The logic here is never designed to be powerful enough >> since it is only a fall-back in core but on Android, we need this >> detection logic to be the primary way of recognizing and playing back >> local content. >> >> 3. Again it would be useful to study the media player comes with >> Android source and make sure that whatever it can recognize and play, >> we can. >> >> fxd >> >> >> Qiang Luo wrote: >>> Thanks for the comments Sheldon. My responses inline. >>> >>> At 05:12 AM 2/19/2009, Sheldon Fu wrote: >>>> Couple of comments, >>>> >>>> 1. The concept of hard-coded 'known extension' in client core is >>>> not very appropriate. Helix DNA can be extended by adding new file >>>> format plugins, which will claim new extensions they support. Also, >>>> build config has choices of which file format plugin to include in >>>> the final installation. There is really not a list of 'known' >>>> extensions to the core, unless you create this list by dynamically >>>> query each file format plugin for extensions they support. The best >>>> we can do here is to skip the detection if there is any extension I >>>> think. >>> >>> The purpose of having this list is to trigger the recognizer's >>> detection process if the given extension is not on the list. This >>> allows us to play something like a_m4a_track.bin instead of just >>> giving it up. If an extension at hand is not on the list but the >>> player has the ff plugin to handle it, then we pay the performance >>> penalty of doing the detection. In general, this is a >>> feature-performance trade off issue. For Android(the change is #if >>> defined() for android), there shouldn't be any problem. We will >>> have a collection of media formats that we must handle, therefor a >>> list of corresponding extensions to skip detection. >>> >>> I will rename function "IsKnownMediaExtension(const char* >>> pFileExtension)" to "ShouldSkipFormatDetection(const char* >>> pFileExtension)". >>> >>>> 2. ID3 isn't always at the front of MP3 file. It could be at the >>>> end and it might not present at all. I am not familiar with other >>>> data types, need to make sure we don't artificially restrict what >>>> we can play here. >>> >>> The format detection code for mp3(and others as well) needs to be >>> improved. In addition to the case you raised above, not all files >>> with ID3 tags are MP3s. I will add code to locate and validate the >>> first frame header. Reliable detection may not be always possible >>> given the limited amount of data(512 byte) at the start of the file. >>> >>> >>>> 3. What about OGG and WAV? >>>> >>>> Since this is the primary code path for playing a local file on >>>> Android, we need to make sure whatever we want to be able to play >>>> will come out of this code with an appropriate mime-type. We should >>>> have a list of data-types we claim we support on Android and we >>>> need to make sure all of them are taken care of here. >>>> >>>> And because we are hoping to be an alternative media engine >>>> solution for Android, we better make sure whatever the current >>>> media engine can play, we can too. E.g, if we only play MP3s with >>>> ID3 tag at the front but current media engine plays when ID3 tag is >>>> at the end or without ID3 tag at all, it won't be a convincing story. >>>> >>>> fxd >>> >>> Tony, could you send me a list of file mime-types that we must >>> support in Android? >>> >>> Thanks, >>> >>> Qiang >>> >>>> Qiang Luo wrote: >>>>> Synopsis: >>>>> Extend CHXFileRecognizer to cover additional mine-types for >>>>> Android project. >>>>> >>>>> Summary: >>>>> In Android project, we are introducing a new protocol scheme, >>>>> fd://, claimed by the local file system, to support file >>>>> descriptor as data source to the helix player. Normally, for the >>>>> local file source, the the client core uses the file extension to >>>>> load the file format plugin via IHXPluginHandler3. When using >>>>> descriptor as data source, the content file name and extension >>>>> will be unknown. Therefore, we need to extend the SDP-only >>>>> CHXFileRecognizer to cover additional mine-types for Android project. >>>>> >>>>> The client core HXFileSource setup code has the following >>>>> mine-type discovery order: >>>>> >>>>> 1) look for "Content-Type" property in the request's response header. >>>>> 2) QI the file-system's file object for IHXFileMimeMapper >>>>> interface and call FindMimeType(). >>>>> 3) get mime-type from url parameters. >>>>> 4) try the file recognizer GetMimeType(m_pFileObject, >>>>> m_pMimeFinderResponse). >>>>> 5) try to create a file format plugin enumerator via >>>>> IHXPluginHandler3 based on the file extension to load the plugin >>>>> 6) fail if none of the above attempts succeeds. >>>>> >>>>> Adding additional mine-types detection to CHXFileRecognizer would >>>>> alter the client core code path. To minimize the risk to other >>>>> projects, I add #if defined() to make the addition Android >>>>> platform only. Even for Android, we defer mime-type detection to >>>>> FileRecognizer only when there is no file-name, or the file >>>>> extension is not a known media type. >>>>> >>>>> Note, the detection code must be simple and small in size. It is >>>>> intended to be just robust enough to load the correct file format >>>>> plugin. >>>>> >>>>> Branch: >>>>> head, hxclient_3_1_0_atlas >>>>> >>>>> File modified: >>>>> common/system/recognizer.cpp >>>>> common/system/pub/recognizer.h >>>>> >>>>> Affected: >>>>> client core. >>>>> >>>>> Risks: >>>>> high. >>>>> >>>>> Please review the attached diff. I'm looking forward to >>>>> suggestions to improve the format detection code. >>>>> >>>>> Thanks, >>>>> >>>>> Qiang >>>>> >>>>> Index: recognizer.cpp >>>>> =================================================================== >>>>> RCS file: /cvsroot/common/system/recognizer.cpp,v >>>>> retrieving revision 1.12 >>>>> diff -u -w -r1.12 recognizer.cpp >>>>> --- recognizer.cpp 7 Feb 2006 19:21:27 -0000 1.12 >>>>> +++ recognizer.cpp 17 Feb 2009 21:55:50 -0000 >>>>> @@ -4,21 +4,116 @@ >>>>> >>>>> #include "recognizer.h" >>>>> >>>>> +// mime-types our recognizer might return >>>>> +#define HX_MIMETYPE_RM "application/x-pn-realmedia" >>>>> +#define HX_MIMETYPE_MP3 "audio/mp3" >>>>> +#define HX_MIMETYPE_MP4AUDIO "audio/mp4" >>>>> +#define HX_MIMETYPE_MP4VIDEO "video/mp4" >>>>> +#define HX_MIMETYPE_3GPAUDIO "audio/3gpp" >>>>> +#define HX_MIMETYPE_3GPVIDEO "video/3gpp" >>>>> +#define HX_MIMETYPE_WM "application/vnd.ms-asf" >>>>> +#define HX_MIMETYPE_SDP "application/sdp" >>>>> + >>>>> +// media file extensions >>>>> +const char* CHXFileRecognizer::zm_pFileExtensions[] = >>>>> +{ >>>>> + "ra", "rm", "rmd", "rmj", "rms", "mnd", "rmc", "rmvb", "mns", >>>>> "mrc", "rax", "rvx", "rv", >>>>> + "mp3", "mp2", "mpa", "mp1", "mpga", "mpg", "mpeg", "mpv", "dat", >>>>> + "mp4", "m4v", "m4a", "mov", "qt" >>>>> + "3gp", "3g2", >>>>> + "asf", "wma", "wmv", >>>>> + NULL >>>>> +}; >>>>> + >>>>> #if !defined (_SYMBIAN) >>>>> HX_RESULT CHXFileRecognizer::GetMimeType(const char* pFileName, >>>>> IHXBuffer* pBuffer, REF(IHXBuffer*) pMimeType) >>>>> { >>>>> + HX_RESULT retVal = HXR_FAILED; >>>>> + char* pMineTypeString = NULL; >>>>> + char* pFileExtension = NULL; >>>>> + CHXString strExtension; >>>>> + HXBOOL bContinue = TRUE; >>>>> + >>>>> + // get file extension >>>>> + if (pFileName) >>>>> + { >>>>> + strExtension = pFileName; >>>>> + INT32 lPeriod = strExtension.ReverseFind('.'); >>>>> + if (lPeriod >= 0) >>>>> + { >>>>> + strExtension = >>>>> strExtension.Right(strExtension.GetLength() - lPeriod - 1); >>>>> + if (strExtension.GetLength() > 0) >>>>> + { >>>>> + strExtension.MakeLower(); >>>>> + pFileExtension = (char*)(const char*)strExtension; >>>>> + } >>>>> + } >>>>> + } >>>>> + >>>>> + // check SDP >>>>> if (IsSDPFile(pBuffer)) >>>>> { >>>>> - if (HXR_OK == CreateBufferCCF(pMimeType, m_pContext)) >>>>> + pMineTypeString = HX_MIMETYPE_SDP; >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> +#if defined(ANDROID) >>>>> + // we don't need to get mime-type by checking the binary data >>>>> + // if the file name has a known media extension. >>>>> + if (bContinue && pFileExtension && >>>>> IsKnownMediaExtension(pFileExtension)) >>>>> + { >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> + // RealMedia >>>>> + if (bContinue && IsRMFile(pBuffer)) >>>>> + { >>>>> + pMineTypeString = HX_MIMETYPE_RM; >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> + // MP3 >>>>> + if (bContinue && IsMP3File(pBuffer)) >>>>> + { >>>>> + pMineTypeString = HX_MIMETYPE_MP3; >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> + // MP4 >>>>> + HXBOOL bVideo = FALSE; >>>>> + if (bContinue && IsMP4File(pBuffer)) >>>>> + { >>>>> + pMineTypeString = HX_MIMETYPE_MP4AUDIO; >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> + // 3GPP >>>>> + if (bContinue && Is3GPPFile(pBuffer)) >>>>> + { >>>>> + pMineTypeString = HX_MIMETYPE_3GPAUDIO; >>>>> + bContinue = FALSE; >>>>> + } >>>>> + >>>>> + // WM >>>>> + if (bContinue && IsWMFile(pBuffer)) >>>>> + { >>>>> + pMineTypeString = HX_MIMETYPE_WM; >>>>> + bContinue = FALSE; >>>>> + } >>>>> +#endif // ANDROID >>>>> + >>>>> + if (pMineTypeString) >>>>> + { >>>>> + retVal = CreateBufferCCF(pMimeType, m_pContext); >>>>> + if (HXR_OK == retVal) >>>>> { >>>>> - int len = strlen("application/sdp"); >>>>> - pMimeType->Set((const UCHAR*)"application/sdp", len + 1); >>>>> + int len = strlen(pMineTypeString); >>>>> + pMimeType->Set((const UCHAR*)pMineTypeString, len + 1); >>>>> ((char*)pMimeType->GetBuffer())[len] = '\0'; >>>>> - return HXR_OK; >>>>> } >>>>> } >>>>> >>>>> - return HXR_FAILED; >>>>> + return retVal; >>>>> } >>>>> #endif >>>>> >>>>> @@ -38,6 +133,124 @@ >>>>> return bResult; >>>>> } >>>>> >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsRMFile(IHXBuffer* pBuffer) >>>>> +{ >>>>> + HXBOOL bResult = FALSE; >>>>> + if (pBuffer && pBuffer->GetSize() > 4) >>>>> + { >>>>> + if (0 == strncmp((const char*)pBuffer->GetBuffer(), >>>>> ".RMF", 4)) >>>>> + { >>>>> + bResult = TRUE; >>>>> + } >>>>> + } >>>>> + return bResult; >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsMP3File(IHXBuffer* pBuffer) >>>>> +{ >>>>> + HXBOOL bResult = FALSE; >>>>> + if (pBuffer && pBuffer->GetSize() > 3) >>>>> + { >>>>> + if (0 == strncmp((const char*)pBuffer->GetBuffer(), >>>>> "ID3", 3)) >>>>> + { >>>>> + //XXX todo: need to check frame header to be sure... >>>>> + bResult = TRUE; >>>>> + } >>>>> + } >>>>> + return bResult; >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsISOMediaFile(IHXBuffer* pBuffer, const char* >>>>> pBrand) >>>>> +{ >>>>> + HXBOOL bResult = FALSE; >>>>> + if (pBuffer) >>>>> + { >>>>> + INT32 nSize = pBuffer->GetSize(); >>>>> + UCHAR* pData = pBuffer->GetBuffer(); >>>>> + UCHAR* pEnd = pData + nSize - 4; >>>>> + >>>>> + // locate file type box >>>>> + pData += 4; >>>>> + HXBOOL bFound = FALSE; >>>>> + while (pData < pEnd) >>>>> + { >>>>> + if ((*pData == 'f' && *(pData+1) == 't' && *(pData+2) == >>>>> 'y' && *(pData+3) == 'p')) >>>>> + { >>>>> + bFound = TRUE; >>>>> + break; >>>>> + } >>>>> + pData++; >>>>> + } >>>>> + >>>>> + if (bFound) >>>>> + { >>>>> + pData += 4; >>>>> + if (0 == strncmp((const char*)pData, pBrand, >>>>> strlen(pBrand))) >>>>> + { >>>>> + bResult = TRUE; >>>>> + } >>>>> + } >>>>> + } >>>>> + return bResult; >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsMP4File(IHXBuffer* pBuffer) >>>>> +{ >>>>> + if (IsISOMediaFile(pBuffer, "M4A") || >>>>> + IsISOMediaFile(pBuffer, "mp4")) >>>>> + { >>>>> + return TRUE; >>>>> + } >>>>> + else >>>>> + { >>>>> + return FALSE; >>>>> + } >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::Is3GPPFile(IHXBuffer* pBuffer) >>>>> +{ >>>>> + return IsISOMediaFile(pBuffer, "3gp"); >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsWMFile(IHXBuffer* pBuffer) >>>>> +{ >>>>> + HXBOOL bResult = FALSE; >>>>> + if (pBuffer) >>>>> + { >>>>> + INT32 nSize = pBuffer->GetSize(); >>>>> + UCHAR* pData = pBuffer->GetBuffer(); >>>>> + UCHAR pMagic[16] = {0x30, 0x26, 0xB2, 0x75, 0x8E, 0x66, 0xCF, >>>>> 0x11, 0xA6, 0xD9, 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C}; >>>>> + if (memcmp(pData, pMagic, 16) == 0) >>>>> + { >>>>> + bResult = TRUE; >>>>> + } >>>>> + } >>>>> + return bResult; >>>>> +} >>>>> + >>>>> +HXBOOL >>>>> +CHXFileRecognizer::IsKnownMediaExtension(const char* pFileExtension) >>>>> +{ >>>>> + if (pFileExtension) >>>>> + { >>>>> + UINT32 i = 0; >>>>> + do { >>>>> + if (strcmp(pFileExtension, zm_pFileExtensions[i]) == 0) >>>>> + { >>>>> + return TRUE; >>>>> + } >>>>> + i++; >>>>> + } while (zm_pFileExtensions[i]); >>>>> + } >>>>> + return FALSE; >>>>> +} >>>>> + >>>>> // length of buffer read from file and passed to recognizer >>>>> static const int KRecogLength = 512; >>>>> >>>>> Index: pub/recognizer.h >>>>> =================================================================== >>>>> RCS file: /cvsroot/common/system/pub/recognizer.h,v >>>>> retrieving revision 1.5 >>>>> diff -u -w -r1.5 recognizer.h >>>>> --- pub/recognizer.h 7 Feb 2006 19:21:27 -0000 1.5 >>>>> +++ pub/recognizer.h 17 Feb 2009 21:55:50 -0000 >>>>> @@ -46,12 +46,21 @@ >>>>> STDMETHOD(WriteDone) (THIS_ HX_RESULT status); >>>>> STDMETHOD(CloseDone) (THIS_ HX_RESULT status); >>>>> >>>>> + static const char* zm_pFileExtensions[]; >>>>> + static HXBOOL IsKnownMediaExtension(const char* >>>>> pFileExtension); >>>>> + >>>>> private: >>>>> STDMETHOD(GetMimeType) (THIS_ const char* /* IN */pFileName, >>>>> IHXBuffer* /* IN */ pBuffer, >>>>> REF(IHXBuffer*) /* OUT*/ pMimeType); >>>>> >>>>> HXBOOL IsSDPFile(IHXBuffer* pBuffer); >>>>> + HXBOOL IsRMFile(IHXBuffer* pBuffer); >>>>> + HXBOOL IsMP3File(IHXBuffer* pBuffer); >>>>> + HXBOOL IsISOMediaFile(IHXBuffer* pBuffer, const char* pBrand); >>>>> + HXBOOL IsMP4File(IHXBuffer* pBuffer); >>>>> + HXBOOL Is3GPPFile(IHXBuffer* pBuffer); >>>>> + HXBOOL IsWMFile(IHXBuffer* pBuffer); >>>>> void DoFileRecognize(void); >>>>> >>>>> >>>>> _______________________________________________ >>>>> Android-port-dev mailing list >>>>> Android-port-dev at lists.helixcommunity.org >>>>> http://lists.helixcommunity.org/mailman/listinfo/android-port-dev >>> >>> > > >