[x265] [PATCH] rc: fixes inconsistent output in linux because of RC Lock issue in CQP/CRF

[Steve Borho]

On 06/27, Deepthi Nandakumar wrote:
> We discussed this, and validated that the inconsistency was due to the
> slice context initializations. 2 FEs do ratecontrolStart in the right order
> but slice context initializations in the wrong order. So the right fix to
> this is to increment the counter after the slice context inits.

if that's all it was, then a simpler fix would probably be sufficient.

> On Jun 27, 2015 12:45 AM, "Steve Borho" <steve at borho.org> wrote:
> 
> > On 06/26, aarthi at multicorewareinc.com wrote:
> > > # HG changeset patch
> > > # User Aarthi Thirumalai
> > > # Date 1434973816 -19800
> > > #      Mon Jun 22 17:20:16 2015 +0530
> > > # Node ID 1c6744174bfcfc031492c041243e1d9a9c02a5cd
> > > # Parent  1e5c4d155ab85e8e8dd199bb3515801766ea9e88
> > > rc: fixes inconsistent output in linux because of RC Lock issue in
> > CQP/CRF
> >
> > It's not clear why this is a race-hazard. Is there something in rate
> > control that must be protected, or is this just covering up for
> > insufficient amounts of ref-lag? What is the race hazard?
> >
> > It's a bit of a layering violation to be mucking about
> > with rate-control internal here, but that can be dealt with in a
> > follow-up patch.
> >
> > > diff -r 1e5c4d155ab8 -r 1c6744174bfc source/encoder/frameencoder.cpp
> > > --- a/source/encoder/frameencoder.cpp Thu Jun 25 13:42:29 2015 +0530
> > > +++ b/source/encoder/frameencoder.cpp Mon Jun 22 17:20:16 2015 +0530
> > > @@ -1056,18 +1056,26 @@
> > >              rowCount = X265_MIN((m_numRows + 1) / 2, m_numRows - 1);
> > >          else
> > >              rowCount = X265_MIN(m_refLagRows, m_numRows - 1);
> > > -        if (row == rowCount)
> > > +    }
> > > +    if (row == rowCount)
> > > +    {
> > > +        m_rce.rowTotalBits = 0;
> > > +        if (m_param->rc.rateControlMode == X265_RC_ABR || bIsVbv)
> > >          {
> > > -            m_rce.rowTotalBits = 0;
> > >              if (bIsVbv)
> > >                  for (uint32_t i = 0; i < rowCount; i++)
> > >                      m_rce.rowTotalBits +=
> > curEncData.m_rowStat[i].encodedBits;
> > >              else
> > >                  for (uint32_t cuAddr = 0; cuAddr < rowCount * numCols;
> > cuAddr++)
> > >                      m_rce.rowTotalBits +=
> > curEncData.m_cuStat[cuAddr].totalBits;
> > > -
> > >              m_top->m_rateControl->rateControlUpdateStats(&m_rce);
> > >          }
> > > +        /* do not allow the next frame to enter rateControlStart()
> > until this
> > > +         * frame has updated its mid-frame statistics */
> > > +        m_top->m_rateControl->m_startEndOrder.incr();
> > > +
> > > +        if (m_rce.encodeOrder < m_param->frameNumThreads - 1)
> > > +            m_top->m_rateControl->m_startEndOrder.incr(); // faked
> > rateControlEnd calls for negative frames
> > >      }
> > >
> > >      /* flush row bitstream (if WPP and no SAO) or flush frame if no WPP
> > and no SAO */
> > > diff -r 1e5c4d155ab8 -r 1c6744174bfc source/encoder/ratecontrol.cpp
> > > --- a/source/encoder/ratecontrol.cpp  Thu Jun 25 13:42:29 2015 +0530
> > > +++ b/source/encoder/ratecontrol.cpp  Mon Jun 22 17:20:16 2015 +0530
> > > @@ -1086,19 +1086,6 @@
> > >          }
> > >      }
> > >      m_framesDone++;
> > > -
> > > -    /* CQP and CRF (without capped VBV) doesn't use mid-frame
> > statistics to
> > > -     * tune RateControl parameters for other frames.
> > > -     * Hence, for these modes, update m_startEndOrder and unlock RC for
> > previous threads waiting in
> > > -     * RateControlEnd here.those modes here. For the rest - ABR
> > > -     * and VBV, unlock only after rateControlUpdateStats of this frame
> > is called */
> > > -    if (m_param->rc.rateControlMode != X265_RC_ABR && !m_isVbv)
> > > -    {
> > > -        m_startEndOrder.incr();
> > > -
> > > -        if (rce->encodeOrder < m_param->frameNumThreads - 1)
> > > -            m_startEndOrder.incr(); // faked rateControlEnd calls for
> > negative frames
> > > -    }
> > >      return m_qp;
> > >  }
> > >
> > > @@ -1625,16 +1612,6 @@
> > >
> > >      m_cplxrSum += rce->rowCplxrSum;
> > >      m_totalBits += rce->rowTotalBits;
> > > -
> > > -    /* do not allow the next frame to enter rateControlStart() until
> > this
> > > -     * frame has updated its mid-frame statistics */
> > > -    if (m_param->rc.rateControlMode == X265_RC_ABR || m_isVbv)
> > > -    {
> > > -        m_startEndOrder.incr();
> > > -
> > > -        if (rce->encodeOrder < m_param->frameNumThreads - 1)
> > > -            m_startEndOrder.incr(); // faked rateControlEnd calls for
> > negative frames
> > > -    }
> > >  }
> > >
> > >  void RateControl::checkAndResetABR(RateControlEntry* rce, bool
> > isFrameDone)
> > > _______________________________________________
> > > x265-devel mailing list
> > > x265-devel at videolan.org
> > > https://mailman.videolan.org/listinfo/x265-devel
> >
> > --
> > Steve Borho
> > _______________________________________________
> > x265-devel mailing list
> > x265-devel at videolan.org
> > https://mailman.videolan.org/listinfo/x265-devel
> >

> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel


-- 
Steve Borho