<div dir="ltr"><div><div><div><div><div>Derek,<br><br></div> 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, <br>
</div><br></div>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<br></div>2. still the work is going on, so unwanted codes, comments and white spaces are removed once the dev completed<br>
<br></div>- Gopu G<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Sep 11, 2013 at 6:41 PM, Derek Buitenhuis <span dir="ltr"><<a href="mailto:derek.buitenhuis@gmail.com" target="_blank">derek.buitenhuis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Sep 11, 2013 at 1:02 PM, Gopu Govindaswamy<br>
<<a href="mailto:gopu@multicorewareinc.com">gopu@multicorewareinc.com</a>> wrote:<br>
> # HG changeset patch<br>
> # User Gopu Govindaswamy <<a href="mailto:gopu@multicorewareinc.com">gopu@multicorewareinc.com</a>><br>
> # Date 1378899885 -19800<br>
> # Node ID 98ee31af129b524005ff8dba503d46b0a83a598b<br>
> # Parent 275f2d5f9b0a782fd3ec36d95268545d96473e2e<br>
> List Implementation<br>
<br>
</div>I understand the point of this is so that STL is not used... but how<br>
exactly is another C++<br>
list implementation at all better than using std::list? This is a<br>
bizarre mix of C-isms and<br>
C++-isms (e.g. new/delete vs alloc/free).<br>
<div class="im"><br>
> -class AccessUnit : public std::list<NALUnitEBSP*><br>
> +<br>
> +/*class AccessUnit : public std::list<NALUnitEBSP*><br>
> {<br>
> public: ~AccessUnit()<br>
> {<br>
> @@ -69,6 +71,32 @@<br>
> delete *it;<br>
> }<br>
> }<br>
> +};*/<br>
<br>
</div>Remove code; don't comment it out.<br>
<div class="im"><br>
> +<br>
> +class AccessUnitn : public List<NALUnitEBSP*><br>
> +{<br>
> +public : ~AccessUnitn()<br>
> + {<br>
<br>
</div>Extra '{' ?<br>
<br>
> + if(this)<br>
<br>
This seems like a good place to ask: What is x265's codign style?<br>
<br>
if(thing) ?<br>
if (thing) ?<br>
if( thing ) ?<br>
if ( thing ) ?<br>
<br>
Same goes for braces, etc.<br>
<div class="im"><br>
> + {<br>
> + NALUnitEBSP *temp;<br>
> + while(this->head != NULL)<br>
> + {<br>
> + temp = (NALUnitEBSP *) this->head->value;<br>
> +<br>
> + printf("%d \n", temp->m_packetSize);<br>
<br>
</div>Left over debug statements?<br>
<div class="im"><br>
> diff -r 275f2d5f9b0a -r 98ee31af129b source/Lib/TLibEncoder/TEncTop.cpp<br>
> --- a/source/Lib/TLibEncoder/TEncTop.cpp Wed Sep 11 12:16:50 2013 +0530<br>
> +++ b/source/Lib/TLibEncoder/TEncTop.cpp Wed Sep 11 17:14:45 2013 +0530<br>
> @@ -113,7 +113,7 @@<br>
> for (int i = 0; i < param.frameNumThreads; i++)<br>
> {<br>
> // Ensure frame encoder is idle before destroying it<br>
> - AccessUnit tmp;<br>
> + AccessUnitn tmp;<br>
<br>
</div>Uh? The 'n' in the few type is for... 'new'... ? :/<br>
<div class="im"><br>
> - accessUnit.insert(accessUnit.end(), new NALUnitEBSP(onalu));<br>
> + accessUnit.push_back(new NALUnitEBSP(onalu));<br>
> }<br>
<br>
</div>Indentation fail.<br>
<div class="im"><br>
> - UInt numRBSPBytes = 0;<br>
> + /* UInt numRBSPBytes = 0;<br>
> for (AccessUnit::const_iterator it = accessUnit.begin(); it != accessUnit.end(); it++)<br>
> {<br>
> UInt numRBSPBytes_nal = (*it)->m_packetSize;<br>
> @@ -558,7 +558,24 @@<br>
> {<br>
> numRBSPBytes += numRBSPBytes_nal;<br>
> }<br>
> - }<br>
> + } */<br>
<br>
</div>See my comment above about commenting out code.<br>
<div class="im"><br>
> diff -r 275f2d5f9b0a -r 98ee31af129b source/Lib/TLibEncoder/TEncTop.h<br>
> --- a/source/Lib/TLibEncoder/TEncTop.h Wed Sep 11 12:16:50 2013 +0530<br>
> +++ b/source/Lib/TLibEncoder/TEncTop.h Wed Sep 11 17:14:45 2013 +0530<br>
> @@ -101,9 +101,9 @@<br>
> void xInitSPS(TComSPS *sps);<br>
> void xInitPPS(TComPPS *pps);<br>
><br>
> - int encode(bool bEos, const x265_picture_t* pic, x265_picture_t *pic_out, AccessUnit& accessUnit);<br>
> + int encode(bool bEos, const x265_picture_t* pic, x265_picture_t *pic_out, AccessUnitn& accessUnit);<br>
><br>
> - int getStreamHeaders(AccessUnit& accessUnit);<br>
> + int getStreamHeaders(AccessUnitn& accessUnit);<br>
<br>
</div>Indentation fail.<br>
<div class="im"><br>
> diff -r 275f2d5f9b0a -r 98ee31af129b source/common/list.h<br>
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000<br>
> +++ b/source/common/list.h Wed Sep 11 17:14:45 2013 +0530<br>
> @@ -0,0 +1,153 @@<br>
<br>
</div>Missing license header comment?<br>
<div class="im"><br>
> +public:<br>
> +<br>
> + Node* head;<br>
> + T size;<br>
> +<br>
> + List()<br>
> + {<br>
> + head = 0;<br>
> + size = 0;<br>
> + }<br>
> +<br>
> + T getSize() { return size; }<br>
<br>
</div>Random style differences.?<br>
<div class="im"><br>
> +<br>
> + inline bool isEmpty()<br>
> + {<br>
> + if(head == NULL)<br>
> + return true;<br>
> + else<br>
> + return false;<br>
> + }<br>
<br>
</div>Aside: IMHO, code in headers is a shitty idea, but that's C++ for you...<br>
<div class="im"><br>
> + }<br>
> +};<br>
> \ No newline at end of file<br>
<br>
</div>Self explanatory.<br>
<br>
> UInt memsize = 0;<br>
<br>
Unrelated to patch: What's with the non-standard int types?<br>
<div class="im"><br>
> - if (encoder->m_nals)<br>
> - X265_FREE(encoder->m_nals);<br>
> + } while(tmp.head->next != NULL && (tmp.head = tmp.head->next));<br>
<br>
</div>Indentation fail? Perhaps I got lost in context.<br>
<div class="im"><br>
> *pp_nal = &encoder->m_nals[0];<br>
> if (pi_nal) *pi_nal = (int)nalcount;<br>
> +<br>
> return 0;<br>
<br>
</div>Random whitespace change.<br>
<div class="im"><br>
> else if (pi_nal)<br>
> *pi_nal = 0;<br>
> +<br>
><br>
> return numEncoded;<br>
<br>
</div>Ditto.<br>
<div class="im"><br>
<br>
> - accessUnit.splice(accessUnit.end(), m_accessUnit);<br>
> + //accessUnit.splice(accessUnit.end(), m_accessUnit);<br>
<br>
</div>See my previous comments.<br>
<div class="im"><br>
> int main(int argc, char *argv[])<br>
> {<br>
> +<br>
> int cpuid = instrset_detect(); // Detect supported instruction set<br>
<br>
</div>Ditto.<br>
<br>
This is mainly a non-technical review. Technical review pending<br>
answers to this review, if it is needed.<br>
<br>
- Derek<br>
_______________________________________________<br>
x265-devel mailing list<br>
<a href="mailto:x265-devel@videolan.org">x265-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x265-devel" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div><br><br clear="all"><br>-- <br>Thanks & Regards<br>Gopu G<br>Multicoreware Inc <br><br>
</div>