<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 12, 2013 at 9:42 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/12/2013 12:46 PM, <a href="mailto:sumalatha@multicorewareinc.com">sumalatha@multicorewareinc.com</a> wrote:<br>

> # HG changeset patch<br>
> # User sumalatha polureddy<br>
> # Date 1378986299 -19800<br>
> # Node ID f8ac7c593a80639220d4c445167383fdaad48dc8<br>
> # Parent  8fdafe573ef7bc2f9ca3eab968f5a563aa06ac54<br>
> 1. Bug fix - improved quality drop for low resolution videos in ABR by 5%<br>
>   - Clipped the qp for the 1st frame to improve quality.<br>
> 2. Changed tuning factors in rateControlEnd from 1.1 to 1.5<br>
>  -  to tune up the actual bits else the cplxrSum is scaled too low to improve short term compensation.<br>
<br>
</div>You should wrap long lines in commit messages, and also have a title line.<br>
<br>
Suggestion:<br>
<br>
    ratecontrol: Tweak to better handle short term compensation<br>
<br>
    Increase the coefficient cplxrSum is adjusted by so that short term<br>
    compensation does not suffer as much.<br>
<br>
    Also, clip the QP for the first frame.<br>
<br>
    Overall improvement is about 5%.<br>
<div class="im"><br>
> +        //introduced to margin QP for first frame to an optimum level in order to stabilize the quality.<br>
<br>
</div>English fix:<br>
<br>
    // Adjust the first frame in order to stabilize the quality level compared to the rest.<br>
<br>
> -        accumPQp = (ABR_INIT_QP)*accumPNorm;<br>
> +        ccumPQp = (ABR_INIT_QP_MIN)*accumPNorm;<br>
<br>
Should really have spaces around the asterisk for consistency.<br>
<div class="im"><br>
> -        cplxrSum = .01 * pow(7.0e5, qCompress) * pow(2 *ncu, 0.5);<br>
> +        cplxrSum = .01 * pow(7.0e5, qCompress) * pow(2 * ncu, 0.5);<br>
<br>
</div>Unrelated changed, technically.<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>Still extra whitespace before the '=='.<br>
<div class="im"><br>
> -<br>
> -        double qScale = qp2qScale(q);<br>
><br>
> -        lastQScaleFor[P_SLICE] = lastQScale = qScale/pbFactor;<br>
> -<br>
> -        return qScale;<br>
> +        return qp2qScale(q);<br>
<br>
</div>Still not entirely sure why this is done....<br>
<br>
The patch is OK on a technical level, I think, though I still do not<br>
understand the above change.<br></blockquote><div><br></div><div><span style="background-color:rgb(255,255,255)"><font color="#0000ff"> The qscale values are clipped(for P frames)  depending on the lastQScaleFor[] at the end to ensure qps dont increase too high/low. they are updated in rateEstimateQScale for non B frames. The above change was introduced so that qp for P frames can adapt based on the qp of  preceding B frames  too -  to see if its improves RateControl. Since it didnt give any big improvements, removing the change.</font></span></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
- Derek<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<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>
</div></div></blockquote></div><br></div></div>