Hi Eric

Thanks for CR. Find my comments inline. The updated code is attached after 
incorporating your suggestions.

I have following 2 queries

- In which .pf file I shall define HELIX_FEATURE_VIDEO_CODEC_AVC1?
- Shall I check-in this code in both cay150 and Head branch?


Regards
Pankaj

At 09:29 PM 10/26/2005, Eric Hyche wrote:

>Pankaj,
>
>Some comments:
>
>1) Why are you not implementing Close()?
>
>     STDMETHOD(Close)            (THIS)  { return HXR_NOTIMPL; }
>
>    It's probably a good idea to implement it. You
>    can put what's in the destructor there and
>    then just call Close() from the destructor.

I followed the code in mp4gpyld.cpp and mp4vpyld.cpp. Added Close function 
as you suggested.


>2) There's no need to declare these inline if you don't
>    have them defined in the .h file. Do these really need
>    to be inline? They don't look like they get called
>    very often...
>
>     inline HX_RESULT SetPacketizerHeader(IHXValues* pHeader);
>     inline HX_RESULT SetAssemblerHeader(IHXValues* pHeader);

inline removed.

>3)     UINT32                   m_lRefCount;
>
>    RefCounts are usually defined to be INT32 instead of UINT32.

changed to INT32.

>4) If you are going to use HX_BITFIELD, then each of these
>    should probably actually be one bit:
>
>     HX_BITFIELD                 m_bFlushed : 1;
>     HX_BITFIELD                 m_bUsesRTPPackets : 1;
>     HX_BITFIELD                 m_bRTPPacketTested : 1;
>     HX_BITFIELD                 m_bPacketize : 1;
>         HX_BITFIELD                     m_bInitialBufferingDone : 1;
>     HX_BITFIELD                 m_bStartPacket : 1;
>         HX_BITFIELD                     m_bFirstPacket : 1;
>         HX_BITFIELD         m_bStartNALFragment : 1;
>         HX_BITFIELD                     m_bFragmentLost : 1;
>
>     HX_BITFIELD just defines to "unsigned char" so each of your
>     booleans were a byte when they could be just a bit. Also,
>     you should probably move the HX_BITFIELDs all to the end
>     of the class definition.

Incorported

>5) In CNALUPacket, does the class own pData? If so, should
>    it get vector deleted upon destruct?

  The pBuffer of CNALUPacket is pointing to the complete buffer of 
IHXPacket and
  pData is pointing to the NALU data received in IHXPacket.  I released the 
pBuffer in
CNALUPacket destructor.  I followed this approach from mp4gpyld.cpp.
Do I need to release in some other way?

>6) In CNALUnit's destructor, you have:
>
>         ~CNALUnit()
>         {
>                 delete pData;
>         }
>
>    Is pData really one byte, or did you mean to
>    do delete [] pData? Also, you should probably use
>    HX_DELETE() or HX_VECTOR_DELETE() macros here.

pData is pointing to a buffer having NALU data. Replaced delete with HX_DELETE.

>7) On your H264PayloadFormat initialization list in the
>    constructor, make sure that the order of member variables
>    matches their declaration. This avoids warnings on some
>    compilers.

Done

>8) Is the allocator a COM object? If so, we should probably
>    hold a ref on it instead of just holding the pointer.

Done

>9) It's probably not a good idea to hold a member variable
>    pointing to a constant string like this:
>
>                 m_pCodecId = "AVC1";
>
>    This looks to be a constant string, so just make it
>    a static const member in the .h file like this:
>
>    static const char* const m_pCodecID;
>
>    and then at the top of the .cpp file, do this:
>
>    const char* const H264PayloadFormat::m_pCodecID = "AVC1";

Done

>10) Please untabify the new files to remove all tabs.

Done

>11) This looks like we are leaking pBuffer:
>
>         // Add this packet to our list of input packets
>         pPacket->AddRef();
>         m_InputQueue.AddTail(pPacket);
>     IHXBuffer * pBuffer = NULL;
>         pBuffer = pPacket->GetBuffer();
>         UINT8* pData = NULL;
>         pData = pBuffer->GetBuffer();
>
>         if (m_bPacketize)
>     {
>         retVal = SetPacketizerPacket(pPacket);
>     }
>     else
>     {
>         retVal = SetAssemblerPacket(pPacket);
>     }
>
>     if (retVal == HXR_NO_DATA)
>     {
>                 retVal = HXR_OK;
>     }
>
>     return retVal;
>
>     When we call pBuffer = pPacket->GetBuffer(), we put
>     a ref on pBuffer (a local variable), but we never
>     release it.

It was a mistake, removed this code. I added pBuffer and pData during my 
testing and forgot to remove.

>12) All the added files should be tri-licensed (RPSL/RPCL/GPL)
>     instead of dual-licensed.

Done

>13) Is the code in base64.h duplicated anywhere in the codebase?

The code is not exactly duplicated. I took it from common\util\perplex.cpp
and modified it according to my need.

>Rest looks good.
>
>Eric
>
> > -----Original Message-----
> > From: datatype-dev-bounces at helixcommunity.org
> > [mailto:datatype-dev-bounces at helixcommunity.org] On Behalf Of
> > Pankaj Gupta
> > Sent: Wednesday, October 26, 2005 7:48 AM
> > To: datatype-dev at helixcommunity.org
> > Cc: psamy at real.com
> > Subject: [datatype-dev] CR: H264 Depacketizer Implementation
> >
> > Modified by: pgupta at real.com
> >
> > Date :10/26/2005
> >
> > Project: H.264 Depacketizer Implementation
> >
> > Synopsis:    Implemetation of H.264 depacketizer described in
> > RTP Payload
> > Format for H.264 Video (RFC 3984)
> >
> > Overview:
> >
> > The RTP payload format allows for packetization of one or
> > more Network
> > Abstraction Layer Units (NALUs), produced by an H.264 video
> > encoder, in
> > each RTP payload.  This module receives packet in RTP payload format
> > containing NAL units. A packet can have a single NAL unit,
> > aggregated NAL
> > units or a fragment of NAL unit. It extracts these NAL units from RTP
> > packet, if it is sent in fragments then collects the fragments to
> > re-generate the original NAL unit. The output of this depacketizer is
> > Access Unit which it produces by collecting the NAL units
> > belongs to same
> > AU bases on same time stamp.  It provides NALU loss handling
> > and if a NALU
> > fragment is lost it ignores complete NALU and informs lost segment in
> > HXOCDEC_DATA.  It also supports all three packetization modes
> >  Single NAL
> > unit mode,  Non-interleaved mode and Interleaved mode.
> > However testing is
> > performed for Single NALU packets and fragmented packets.
> >
> > The integration of this code into the mp4 video renderer is
> > conditional on
> > HELIX_FEATURE_VIDEO_CODEC_AVC1 .
> >
> > Platform: windows
> >
> > Branch : HEAD, 150Cay
> >
> > File Modified:
> > datatype/mp4/payload/Umakefil
> > datatype/mp4/payload/fmtfact.cpp
> > datatype/mp4/payload/mp4vpyld.cpp
> > datatype/mp4/payload/pub/fmtfact.h
> > datatype/mp4/payload/pub/mp4vpyld.h
> > datatype/mp4/video/renderer/mp4vdfmt.cpp
> > datatype/mp4/video/renderer/mp4video.cpp
> > datatype/mp4/video/renderer/pub/mp4vdfmt.h
> >
> > Files Added
> > h264pyld.cpp
> > h264pyld.h
> > mp4vpyif.h
> > base64.h
> >
> > Thanks and regards,
> > Pankaj Gupta
> >
> > CVS Diff:
> >
> > Attached H264DepacketizeDiff.zip
> >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: H264DepacketizerDiff_27_10_2005.zip
Type: application/octet-stream
Size: 20393 bytes
Desc: not available
Url : http://lists.helixcommunity.org/pipermail/datatype-dev/attachments/20051027/afd58e98/H264DepacketizerDiff_27_10_2005-0001.obj
-------------- next part --------------