[datatype-dev] Adding support for H.264 in AVI file format
Rishi Mathew rmathew at real.comIs the FourCC look up case-insensitive? I have seen at least a few files with mixed-case 4 cc? Would the lookup fail in that case and return a NO_Fileformat error? http://www.fourcc.org/codecs.php Also, as an aside, the 4cc for DIVX 5.0 should be "DX50" instead of "DV50", but that is probably for another CR. Cheers, Rishi. At 05:19 PM 12/23/2008, Jyotsana Rathore wrote: >Thanks for the review Greg. >This is now checked into 310Atlas and HEAD. > >Thanks, >Jyotsana > >-----Original Message----- >From: Gregory Wright [mailto:gwright at real.com] >Sent: Tuesday, December 23, 2008 3:35 PM >To: Jyotsana Rathore >Cc: ehyche at real.com; datatype-dev at helixcommunity.org >Subject: Re: [datatype-dev] Adding support for H.264 in AVI file format > >Looks good. >--greg. > >On Dec 22, 2008, at 10:51 AM, Jyotsana Rathore wrote: > > > Hi Eric, > > > > Attached is an updated diff. The only difference to the previous one > > is that > > I have updated the comments and added reference to the ISO/ IEC > > 14496-10 > > (Advanced Video Coding) standard. > > > > Thanks, > > Jyotsana > > > > -----Original Message----- > > From: datatype-dev-bounces at helixcommunity.org > > [mailto:datatype-dev-bounces at helixcommunity.org] On Behalf Of Jyotsana > > Rathore > > Sent: Friday, December 19, 2008 1:35 PM > > To: ehyche at real.com; datatype-dev at helixcommunity.org > > Subject: RE: [datatype-dev] Adding support for H.264 in AVI file > > format > > > > Hi Eric, > > > > My Response is inlined. > > Updated diff is attached. > > > > Thanks, > > Jyotsana > > > > -----Original Message----- > > From: Eric Hyche [mailto:ehyche at real.com] > > Sent: Friday, December 19, 2008 11:24 AM > > To: 'Jyotsana Rathore'; datatype-dev at helixcommunity.org > > Subject: RE: [datatype-dev] Adding support for H.264 in AVI file > > format > > > > Jyotsana: > > > > Here are my comments: > > > > - avistrm.cpp changes look good > > - avcconfig.cpp - this change doesn't look necessary to > > me. If seems like that AVCConfigurationBox::Unpack() > > returns failure if it doesn't find a avcC box already. > > Why are the changes in avcconfig.cpp, avcconfig.h, > > and mp4vpyld.cpp necessary? > > [Jyotsana] The only problem is that the retVal will otherwise be set > > to > > HXR_INVALID_PARAMETER which will throw me out completely. If its > > better, > > then I can remove all my changes in these files and mask the error by > > setting retval = HXR_OK before exiting from > > SetAssemblerHX3GPPH264Header > > function in mp4vpyld.cpp That also works. > > > > - mp4vdec.cpp changes look good > > - avc1dstm.cpp > > > > + m_bHasAVCC = true; > > > > This should be TRUE, not the C++ keyword "true". > > > > + }else > > + { > > > > [Jyotsana] OK. Done > > > > Helix coding conventions would put the "else" on > > its own line, like this: > > > > } > > else > > { > > [Jyotsana] Ok. Done > > > > + //This is the case when there is no > > AVCConfigurationRecord > > + //And thus do not have m_ulNALUSizeField. The number of > > bytes > > to > > + //be decoded have to be computed differently > > > > Please put a more descriptive comment about what the > > code below this comment is doing. More background - like > > under what conditions will an AVCConfigurationRecord not > > be found (i.e - H.264 in AVI). Also, if the "number of bytes > > to be decoded have to be computed differently", then please > > explain the difference. References to the appropriate sections > > in the MPEG AVC standard would be great. > > > > Descriptive comments will greatly aid anyone examining the code > > later. > > > > [Jyotsana] I have added some more comments but unfortunately I don't > > have > > MPEG AVC references. This is something I figured out from FFMpeg > > library. > > > > > > @@ -982,11 +1025,11 @@ > > } > > > > // Take custom SAR into account > > - if (m_sequence_parameters.vui_parameters && > > - > > m_sequence_parameters.vui_parameters->aspect_ratio_info_present_flag) > > + if (sequence_parameters->vui_parameters && > > + > > sequence_parameters->vui_parameters->aspect_ratio_info_present_flag) > > { > > - sar_width = > > m_sequence_parameters.vui_parameters->sar_width; > > - sar_height = > > m_sequence_parameters.vui_parameters->sar_height; > > + sar_width = > > sequence_parameters->vui_parameters->sar_width; > > + sar_height = > > sequence_parameters->vui_parameters->sar_height; > > m_mofYUV.uiWidth = > > (unsigned int)(m_mofYUV.uiWidth * > > ((float)sar_width > > / (float)sar_height)); > > if (!m_bWidthHeightKnown) > > > > I don't actually see any changes here. You may have done > > a "diff -u" instead of a "diff -uw", so these may be > > just whitespace differences. The "-w" argument to "diff" > > will ignore whitespace changes. > > > > [Jyotsana] The change is sequence_parameters instead of > > m_sequence_parameters. Since m_sequence_parameters is set in > > AVCDecoderConfigurationRecord function which is not executed in case > > of > > H.264 in AVI I am using sequence_parameters which is obtained from the > > decoder_parameters. They are same. > > > > Rest looks good. > > > > Eric > > > > ======================================= > > Eric Hyche (ehyche at real.com) > > Principal Engineer > > RealNetworks, Inc. > > > > > >> -----Original Message----- > >> From: datatype-dev-bounces at helixcommunity.org > > [mailto:datatype-dev-bounces at helixcommunity.org] On > >> Behalf Of Jyotsana Rathore > >> Sent: Thursday, December 18, 2008 4:43 PM > >> To: datatype-dev at helixcommunity.org > >> Subject: [datatype-dev] Adding support for H.264 in AVI file format > >> > >> Modified by: jrathore at real.com > >> > >> Date: 12/18/08 > >> > >> Project: Helix DNA Client > >> > >> > >> > >> Synopsis: Adding support for H.264 in AVI file format > >> > >> > >> > >> Overview: This change enables playback of H.264 video inside an AVI > >> file. > > There is no AVC Decoder > >> Configuration Record found in such type of files and the Sequence > >> Parameter > > Set (SPS) and Picture > >> Parameter Set (PPS) are within the frames. However this means that > >> we do > > not have the NALUSizeField > >> available and we have to compute the appropriate number of bytes for > > decoding differently. > >> > >> > >> > >> Files Added: None > >> > >> > >> > >> Files Modified: > >> > >> avistrm.cpp (/datatype/avi/fileformat/avistrm.cpp) > >> > >> avcconfig.cpp (/datatype/mp4/common/avcconfig.cpp) > >> > >> avcconfig.h (/datatype/mp4/common/pub/avcconfig.h) > >> > >> mp4vpyld.cpp (/datatype/mp4/payload/mp4vpyld.cpp) > >> > >> mp4vdec.cpp (/datatype/mp4/video/renderer/mp4vdec.cpp) > >> > >> avc1dstm.cpp (datatype-restricted/h264/codec/decoder/frontend/ > >> avc1dstm.cpp) > >> > >> avc1dstm.h (datatype-restricted/h264/codec/decoder/frontend/pub/ > >> avc1dstm.h) > >> > >> > >> > >> Image Size and Heap Use impact (Client -Only): None > >> > >> > >> > >> Platforms and Profiles Affected: > >> > >> Platform: win32-i386-vc7 > >> > >> Profile: helix-client-all-defines > >> > >> > >> > >> Platforms and Profiles Build and Functionality Verified: > >> > >> Platform: win32-i386-vc7 > >> > >> Profile: helix-client-all-defines > >> > >> target(s): splay > >> > >> > >> > >> Branch: hxclient_3_1_0_atlas, HEAD > >> > >> > >> > >> Copyright assignment: I am a RealNetworks employee. > >> > >> > >> > >> Files Attached: avc-in-avi.diff > >> > >> > >> > >> -Jyotsana > >> > >> > > > > <avc-in- > > avi_updated.diff>_______________________________________________ > > Datatype-dev mailing list > > Datatype-dev at helixcommunity.org > > http://lists.helixcommunity.org/mailman/listinfo/datatype-dev > > >_______________________________________________ >Datatype-dev mailing list >Datatype-dev at helixcommunity.org >http://lists.helixcommunity.org/mailman/listinfo/datatype-dev Rishi Mathew Helix Community RealNetworks, Inc. rmathew at real.com http://www.helixcommunity.org http://www.realnetworks.com/products/support/devsupport.html -------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.helixcommunity.org/pipermail/datatype-dev/attachments/20081224/f7aef646/attachment-0001.html