[x265] [PATCH RFC ] List Implementation

[Derek Buitenhuis]

On Wed, Sep 11, 2013 at 1:02 PM, Gopu Govindaswamy
<gopu at multicorewareinc.com> wrote:
> # HG changeset patch
> # User Gopu Govindaswamy <gopu at multicorewareinc.com>
> # Date 1378899885 -19800
> # Node ID 98ee31af129b524005ff8dba503d46b0a83a598b
> # Parent  275f2d5f9b0a782fd3ec36d95268545d96473e2e
> List Implementation

I understand the point of this is so that STL is not used... but how
exactly is another C++
list implementation at all better than using std::list? This is a
bizarre mix of C-isms and
C++-isms (e.g. new/delete vs alloc/free).

> -class AccessUnit : public std::list<NALUnitEBSP*>
> +
> +/*class AccessUnit : public std::list<NALUnitEBSP*>
>  {
>  public: ~AccessUnit()
>      {
> @@ -69,6 +71,32 @@
>              delete *it;
>          }
>      }
> +};*/

Remove code; don't comment it out.

> +
> +class AccessUnitn : public List<NALUnitEBSP*>
> +{
> +public : ~AccessUnitn()
> +       {

Extra '{' ?

> +               if(this)

This seems like a good place to ask: What is x265's codign style?

if(thing) ?
if (thing) ?
if( thing ) ?
if ( thing ) ?

Same goes for braces, etc.

> +               {
> +                       NALUnitEBSP *temp;
> +                       while(this->head != NULL)
> +                       {
> +                               temp = (NALUnitEBSP *) this->head->value;
> +
> +                               printf("%d \n", temp->m_packetSize);

Left over debug statements?

> diff -r 275f2d5f9b0a -r 98ee31af129b source/Lib/TLibEncoder/TEncTop.cpp
> --- a/source/Lib/TLibEncoder/TEncTop.cpp        Wed Sep 11 12:16:50 2013 +0530
> +++ b/source/Lib/TLibEncoder/TEncTop.cpp        Wed Sep 11 17:14:45 2013 +0530
> @@ -113,7 +113,7 @@
>          for (int i = 0; i < param.frameNumThreads; i++)
>          {
>              // Ensure frame encoder is idle before destroying it
> -            AccessUnit tmp;
> +            AccessUnitn tmp;

Uh? The 'n' in the few type is for... 'new'... ? :/

> -        accessUnit.insert(accessUnit.end(), new NALUnitEBSP(onalu));
> +               accessUnit.push_back(new NALUnitEBSP(onalu));
>      }

Indentation fail.

> -    UInt numRBSPBytes = 0;
> +  /*  UInt numRBSPBytes = 0;
>      for (AccessUnit::const_iterator it = accessUnit.begin(); it != accessUnit.end(); it++)
>      {
>          UInt numRBSPBytes_nal = (*it)->m_packetSize;
> @@ -558,7 +558,24 @@
>          {
>              numRBSPBytes += numRBSPBytes_nal;
>          }
> -    }
> +    } */

See my comment above about commenting out code.

> diff -r 275f2d5f9b0a -r 98ee31af129b source/Lib/TLibEncoder/TEncTop.h
> --- a/source/Lib/TLibEncoder/TEncTop.h  Wed Sep 11 12:16:50 2013 +0530
> +++ b/source/Lib/TLibEncoder/TEncTop.h  Wed Sep 11 17:14:45 2013 +0530
> @@ -101,9 +101,9 @@
>      void xInitSPS(TComSPS *sps);
>      void xInitPPS(TComPPS *pps);
>
> -    int encode(bool bEos, const x265_picture_t* pic, x265_picture_t *pic_out, AccessUnit& accessUnit);
> +    int encode(bool bEos, const x265_picture_t* pic, x265_picture_t *pic_out, AccessUnitn& accessUnit);
>
> -    int getStreamHeaders(AccessUnit& accessUnit);
> +       int getStreamHeaders(AccessUnitn& accessUnit);

Indentation fail.

> diff -r 275f2d5f9b0a -r 98ee31af129b source/common/list.h
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/source/common/list.h      Wed Sep 11 17:14:45 2013 +0530
> @@ -0,0 +1,153 @@

Missing license header comment?

> +public:
> +
> +       Node* head;
> +       T size;
> +
> +       List()
> +          {
> +                  head = 0;
> +                  size = 0;
> +          }
> +
> +       T getSize() { return size; }

Random style differences.?

> +
> +          inline bool isEmpty()
> +          {
> +                  if(head == NULL)
> +                          return true;
> +                  else
> +                          return false;
> +          }

Aside: IMHO, code in headers is a shitty idea, but that's C++ for you...

> +         }
> +};
> \ No newline at end of file

Self explanatory.

>          UInt memsize = 0;

Unrelated to patch: What's with the non-standard int types?

> -        if (encoder->m_nals)
> -            X265_FREE(encoder->m_nals);
> +               } while(tmp.head->next != NULL && (tmp.head = tmp.head->next));

Indentation fail? Perhaps I got lost in context.

>          *pp_nal = &encoder->m_nals[0];
>          if (pi_nal) *pi_nal = (int)nalcount;
> +
>          return 0;

Random whitespace change.

>      else if (pi_nal)
>          *pi_nal = 0;
> +
>
>      return numEncoded;

Ditto.


> -        accessUnit.splice(accessUnit.end(), m_accessUnit);
> +        //accessUnit.splice(accessUnit.end(), m_accessUnit);

See my previous comments.

>  int main(int argc, char *argv[])
>  {
> +
>      int cpuid = instrset_detect(); // Detect supported instruction set

Ditto.

This is mainly a non-technical review. Technical review pending
answers to this review, if it is needed.

- Derek