<p dir="ltr">Should m_startEndOrder be a member of Encoder, rather than ratecontrol, since it serializes RcStarts, slice context inits as well as   some CTU row encodes? </p>
<div class="gmail_quote">On Jun 29, 2015 9:21 PM, "Steve Borho" <<a href="mailto:steve@borho.org">steve@borho.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 06/29, <a href="mailto:aarthi@multicorewareinc.com">aarthi@multicorewareinc.com</a> wrote:<br>
> # HG changeset patch<br>
> # User Aarthi Thirumalai<br>
> # Date 1435312791 -19800<br>
> #      Fri Jun 26 15:29:51 2015 +0530<br>
> # Node ID 2dac97717742d6187df7798bdfbd8a9f04e6f446<br>
> # Parent  f7bbb04e1992a238aee739a0e21777932c1dfa57<br>
> rc: fixes inconsistent output in linux because of RC Lock in CQP/CRF<br>
><br>
> the inconsistency is due to a race hazrd in slice context initializations when 2 Frame Encoders complete<br>
> RateControlStart in correct order but slice context initializations in the wrong order in CRF/CQP.<br>
<br>
Ok.<br>
<br>
So for CRF and CQP instead of auto-incrementing m_startEndOrder just<br>
before exiting the rateControlStart() function, the 'end' signal is<br>
deffered until just before FE enters the CTU compression loop, thus all<br>
slice setup work becomes serialized between frame encoders.<br>
<br>
makes sense. but my older comment stands in that this is somewhat of a<br>
layering violation. but I don't have any good ideas right now about how<br>
to improve it.<br>
<br>
I hope m_startEndOrder has a decent comment somewhere which describes<br>
that it is serializing certain portions of the frame encoder main loop<br>
to protect rate control and/or slice initialization from race hazards.<br>
And how, for ABR, it even serializes a certain number of CTU rows (up to<br>
rateControlUpdateStats) in order to improve ABR efficiency.<br>
<br>
> diff -r f7bbb04e1992 -r 2dac97717742 source/encoder/frameencoder.cpp<br>
> --- a/source/encoder/frameencoder.cpp Fri Jun 26 10:16:29 2015 +0530<br>
> +++ b/source/encoder/frameencoder.cpp Fri Jun 26 15:29:51 2015 +0530<br>
> @@ -474,6 +474,19 @@<br>
>          m_nalList.serialize(NAL_UNIT_PREFIX_SEI, m_bs);<br>
>      }<br>
><br>
> +    /* CQP and CRF (without capped VBV) doesn't use mid-frame statistics to<br>
> +     * tune RateControl parameters for other frames.<br>
> +     * Hence, for these modes, update m_startEndOrder and unlock RC for previous threads waiting in<br>
> +     * RateControlEnd here, after the slicecontexts are initialized. For the rest - ABR<br>
> +     * and VBV, unlock only after rateControlUpdateStats of this frame is called */<br>
> +    if (m_param->rc.rateControlMode != X265_RC_ABR && !m_top->m_rateControl->m_isVbv)<br>
> +    {<br>
> +        m_top->m_rateControl->m_startEndOrder.incr();<br>
> +<br>
> +        if (m_rce.encodeOrder < m_param->frameNumThreads - 1)<br>
> +            m_top->m_rateControl->m_startEndOrder.incr(); // faked rateControlEnd calls for negative frames<br>
> +    }<br>
> +<br>
>      /* Analyze CTU rows, most of the hard work is done here.  Frame is<br>
>       * compressed in a wave-front pattern if WPP is enabled. Row based loop<br>
>       * filters runs behind the CTU compression and reconstruction */<br>
> diff -r f7bbb04e1992 -r 2dac97717742 source/encoder/ratecontrol.cpp<br>
> --- a/source/encoder/ratecontrol.cpp  Fri Jun 26 10:16:29 2015 +0530<br>
> +++ b/source/encoder/ratecontrol.cpp  Fri Jun 26 15:29:51 2015 +0530<br>
> @@ -1087,18 +1087,6 @@<br>
>      }<br>
>      m_framesDone++;<br>
><br>
> -    /* CQP and CRF (without capped VBV) doesn't use mid-frame statistics to<br>
> -     * tune RateControl parameters for other frames.<br>
> -     * Hence, for these modes, update m_startEndOrder and unlock RC for previous threads waiting in<br>
> -     * RateControlEnd here.those modes here. For the rest - ABR<br>
> -     * and VBV, unlock only after rateControlUpdateStats of this frame is called */<br>
> -    if (m_param->rc.rateControlMode != X265_RC_ABR && !m_isVbv)<br>
> -    {<br>
> -        m_startEndOrder.incr();<br>
> -<br>
> -        if (rce->encodeOrder < m_param->frameNumThreads - 1)<br>
> -            m_startEndOrder.incr(); // faked rateControlEnd calls for negative frames<br>
> -    }<br>
>      return m_qp;<br>
>  }<br>
><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" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
<br>
--<br>
Steve Borho<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" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x265-devel</a><br>
</blockquote></div>