[x265] [PATCH] encoder: do not use std::list

[Jean-Baptiste Kempf]

Le 18/09/2013 13:47, Gopu Govindaswamy a écrit :
>
> On Wed, Sep 18, 2013 at 5:05 PM, Jean-Baptiste Kempf <jb at videolan.org
> <mailto:jb at videolan.org>> wrote:
>
>     On 18 Sep, Gopu Govindaswamy wrote :
>      > -
>      > +
>
>     whitespace issue.
>      > -            AccessUnit tmp;
>      > +           NALUnitEBSP **tmp = NULL;
>
>     Identation is off
>     Identation ?

Number of whitespaces

>      > @@ -169,7 +169,7 @@
>      >   \param   accessUnitsOut      output bitstream
>      >   \retval                      number of encoded pictures
>      >   */
>      > -int TEncTop::encode(bool flush, const x265_picture_t* pic_in,
>     x265_picture_t *pic_out, AccessUnit& accessUnitOut)
>      > +int TEncTop::encode(bool flush, const x265_picture_t* pic_in,
>     x265_picture_t *pic_out, NALUnitEBSP **nalunits)
>
>     You fail to update the doxygen doc.
>     I will update into doxygen doc

It should be in the same patch though...

>
>      > -        accessUnit.insert(accessUnit.end(), new NALUnitEBSP(onalu));
>      > +        //accessUnit.insert(accessUnit.end(), new
>     NALUnitEBSP(onalu));
>
>     Why?
>     forgot to modify this place, after sent the patch I have sent the
>     separate patch for it,

You should split those commits, if you can.

>      >      UInt numRBSPBytes = 0;
>      > -    for (AccessUnit::const_iterator it = accessUnit.begin(); it
>     != accessUnit.end(); it++)
>      > +    int count = 0;
>      > +    while (nalunits[count])
>
>     Are you sure nalunits[0] always exists?
>     Yes i am sure nalunits[0] to nalunits[5] has 0 , i have initialized
>     the nalunit in declaration

OK.

>      >      {
>      > -        UInt numRBSPBytes_nal = (*it)->m_packetSize;
>      > +        UInt numRBSPBytes_nal = nalunits[count]->m_packetSize;
>      >  #if VERBOSE_RATE
>      >          printf("*** %6s numBytesInNALunit: %u\n",
>     nalUnitTypeToString((*it)->m_nalUnitType), numRBSPBytes_nal);
>      >  #endif
>      > -        if ((*it)->m_nalUnitType != NAL_UNIT_PREFIX_SEI &&
>     (*it)->m_nalUnitType != NAL_UNIT_SUFFIX_SEI)
>      > +        if (nalunits[count]->m_nalUnitType !=
>     NAL_UNIT_PREFIX_SEI && nalunits[count]->m_nalUnitType !=
>     NAL_UNIT_SUFFIX_SEI)
>      >          {
>      >              numRBSPBytes += numRBSPBytes_nal;
>      >          }
>      > +        count++;
>      >      }
>
>     Why not keeping the for syntax here?
>     syntax?

You transform the "for" into a "while", but you keep having a count++,

for( ; nalunits[count] != NULL; count++ )
would be a smaller change

>      > +    NALUnitEBSP *nalunits[5] = { 0, 0, 0, 0, 0 };
>
>     You should make this 5 a define, so you can use it later on
>
>      >          if (pi_nal) *pi_nal = (int)nalcount;
>      > +
>
>     White space
>
>      > +    NALUnitEBSP *nalunits[5] = { 0, 0, 0, 0, 0 };
>      > +    int numEncoded = encoder->encode(!pic_in, pic_in, pic_out,
>     nalunits);
>      > +
>
>     idem
>     idem?

Idem, aka, same remark as above, aka whitespace

>      > +            m_accessUnit[m_nalcount] = (NALUnitEBSP
>     *)X265_MALLOC(NALUnitEBSP, 1);
>
>     Why don't you check your malloc return???
>
>      X265_MALLOC Will take care of this

No.

And you should really change your Mail Client to use proper quoting.

Best

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device