[datatype-dev] Adding support for H.264 in AVI file format
Jyotsana Rathore jrathore at real.comHi 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 > > -------------- next part -------------- A non-text attachment was scrubbed... Name: avc-in-avi_updated.diff Type: application/octet-stream Size: 5186 bytes Desc: not available Url : http://lists.helixcommunity.org/pipermail/datatype-dev/attachments/20081222/5efaefc6/avc-in-avi_updated.obj