[x265] [PATCH RFC ] List Implementation
Gopu Govindaswamy
gopu at multicorewareinc.com
Thu Sep 12 08:29:34 CEST 2013
Derek,
Thanks for review and comments, this patch was just to tell the approach
for STL replacement in our x265, in x265 we are using very few std::list
API's, so we are trying to replace those API's with our own implementation,
1. in list.h we will not have all the std::list API's implementations, we
will have very few thats limited to the x265
2. still the work is going on, so unwanted codes, comments and white spaces
are removed once the dev completed
- Gopu G
On Wed, Sep 11, 2013 at 6:41 PM, Derek Buitenhuis <
derek.buitenhuis at gmail.com> wrote:
> 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
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel
>
--
Thanks & Regards
Gopu G
Multicoreware Inc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20130912/19c565e5/attachment-0001.html>
More information about the x265-devel
mailing list