[x265] [PATCH] Make FrameEncoder partially virtual so it can be overloaded

[Steve Borho]

On 02/03, Nicolas Morey-Chaisemartin wrote:
> 
> On 02/02/2015 06:27 PM, Steve Borho wrote:
> >On 02/02, Nicolas Morey-Chaisemartin wrote:
> >># HG changeset patch
> >># User Nicolas Morey-Chaisemartin <nmorey at kalray.eu>
> >># Date 1414489826 -3600
> >>#      Tue Oct 28 10:50:26 2014 +0100
> >>
> >>Make FrameEncoder partially virtual so it can be overloaded
> >>
> >>FrameEncoder is a logical place to use a hardware accelerator or an alternative core encoder.
> >>By making a few function virtual, it can be easily replaced by an inheriting class, transparently for the Encoder.
> >>This way all the RC/interaction code can stay within the FrameEncoder class and reduce conflicts in later updates.
> >I'm mostly fine with adding virtual declarations for these functions,
> >but I'm curious why this requires the frame encoders to be allocated in
> >seperate new() calls? This is accounting for most of the diffs.
> >
> >Somewhat related to this, I made a patch a couple of days ago that has
> >yet to be pushed which added a hard-limit on the number of frame
> >encoders (to 16). With this you could avoid mallocing the array of
> >pointers, and just declare the max-sized array in the top level Encoder
> >class (if individual pointers are required).
> >
> >
> This is required to handle multiple object sizes.
> Because Encoder sees FrameEncoder has an interface, it has no real idea what the exact size of object implementing the interface is (except at the moment of allocation).
> This means that every class implementing FrameEncoder interface must have the exact same size and thus no extra member/virtual functions...
> Otherwise, you're sure to get a SEGV somewhere in there.
> 
> With my proposal, the only requirement to switch to a new FrameEncoder is to add something like this for the instantiation
>    for (int i = 0; i < m_param->frameNumThreads; i++)
>     {
>         if (m_param->accelerationType != X265_ACCEL_NONE)
>             m_frameEncoder[i] = new FrameEncoderKalray;
>         else
>             m_frameEncoder[i] = new FrameEncoder;
> 
>         m_frameEncoder[i]->setThreadPool(m_threadPool);
>     }

m_frameEncoder = new FrameEncoderKalray[m_param->frameNumThreads] should
still work unless you plan to use a mixture of FrameEncoder and
FrameEncoderKalray.

Either way, can you rebase your change to the new tip and use
FrameEncoder*  m_frameEncoder[X265_MAX_FRAME_THREADS] in the top-level
Encoder?

-- 
Steve Borho