<div dir="ltr">Thanks for comments. Noted. Adding the correction suggested and a few more tweaks in the next commit. <div><br></div><div>--Aarthi</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 12, 2013 at 3:28 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 9/11/2013 5:31 PM, <a href="mailto:aarthi@multicorewareinc.com">aarthi@multicorewareinc.com</a> wrote:<br>

> # HG changeset patch<br>
> # User Aarthi Thirumalai<br>
> # Date 1378916973 -19800<br>
> #      Wed Sep 11 21:59:33 2013 +0530<br>
> # Node ID 823b35128a7dcbd77f0d4c274a285d63deba7c5a<br>
> # Parent  a6bf1f10e820da79da76b4649eaae11b6a030521<br>
> modify for BFrames in ABR , clean up code<br>
<br>
</div>Perhaps:<br>
<br>
    Improve bframe ratecontrol logic<br>
<br>
    This should improve ABR results by <...>.<br>
<br>
Where <...> is an explanation on how it improves it. I understand<br>
it's mostly tweaking magic numbers.<br>
<div class="im"><br>
> +        TComSlice* prevRefSlice = curFrame->getRefPic(REF_PIC_LIST_0, 0)->getSlice();<br>
> +        TComSlice* nextRefSlice = curFrame->getRefPic(REF_PIC_LIST_1, 0)->getSlice();<br>
> +        int i0 = prevRefSlice->getSliceType() == I_SLICE;<br>
> +        int i1 = nextRefSlice->getSliceType() == I_SLICE;<br>
<br>
</div>bool? Or do we intentionally stick to ints to make a C port<br>
easier later on?<br>
<div class="im"><br>
> +        if (prevRefSlice->getSliceType()  == B_SLICE && prevRefSlice->isReferenced())<br>
> +            q0 -= pbOffset / 2;<br>
> +        if (nextRefSlice->getSliceType()  == B_SLICE && nextRefSlice->isReferenced())<br>
> +            q1 -= pbOffset / 2;<br>
<br>
</div>Accidental extra whitespace before the ==.<br>
<div class="im"><br>
> -        lastQScaleFor[P_SLICE] = lastQScale = qScale/pbFactor;<br>
<br>
</div>Can you document the reason for removing this in the commit message?<br>
<div class="im"><br>
> -            cplxrSum +=  1.1 *bits * qp2qScale(rce->qpaRc) / rce->lastRceq;<br>
> +            /* The factor 1.5 is to tune up the actual bits  otherwise the cplxrSum is scaled too low<br>
> +             * to improve short term compensation for next frame. */<br>
> +            cplxrSum +=  1.5 * bits * qp2qScale(rce->qpaRc) / rce->lastRceq;<br>
<br>
</div>Extra space after 'bits', where a comma should be.<br>
<br>
Also, s/the cplxrSum/cplxrSum/.<br>
<div class="im"><br>
> -            cplxrSum += bits * qp2qScale(rce->qpaRc) / (rce->lastRceq * fabs(0.5 * pbFactor));<br>
> +            cplxrSum += bits * qp2qScale(rce->qpaRc) / (rce->lastRceq * fabs(pbFactor));<br>
<br>
</div>I assume this is related to the above tweak, i.e. a natrual consequence of<br>
increasing the coefficient cplxrSum uses?<br>
<br>
Technically, seems OK.<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></div>