[FFmpeg-devel] [PATCH] 10-bit DNxHD support v4 review request

[Baptiste Coudurier]

On 4/7/11 1:56 AM, Joseph Artsimovich wrote:
> Baptiste Coudurier wrote:
> 
>>>> bit_depth is a better name IMHO.
>>> Well, this field was meant to indicate how the DSPContext was
>>> initialized (if
>> at all).
>>> For other purposes we have cid_table->bit_depth
>> 
>> I think you can check the current cid bit depth if set already.
> 
> The problem is that CID comes from the frame header, and therefore
> theoretically can change in the middle of a stream.  I know that's
> not currently supported, but at least my approach doesn't make it
> harder to add such support in the future.  I've evaluated a couple of
> alternative approaches and they either lead to  ugly and confusing
> code or require refactoring of existing code.

You can check for the previous cid set in the context, if it differs,
reinit dsp.

>>>>> +static av_always_inline void
>>>>> dnxhd_10bit_get_pixels_8x8(DCTELEM +*restrict block, const
>>>>> uint16_t *pixels, int line_size) { +    for (int i = 0; i <
>>>>> 8; ++i) { +        for (int j = 0; j < 8; ++j) { +
>>>>> block[j] = (DCTELEM)floor((float)pixels[j] * (1023.0f / 
>>>>> +65535.0f) + 0.5f);
>>>> 
>>>> Btw, why are you using floats here ?
>>> Hoping a compiler might vectorize this code.
>> 
>> We prefer using ints here, to avoid rounding difference on
>> different archs.
> 
> I heard integer divison is quite expensive - more expensive than
> floating point multiplication + rounding. Note that we are dividing
> by (2^16 - 1) not by 2^16, so it can't be replaced by a shift. As for
> rounding, in the latest version of my patch I use lrintf(). This
> article http://www.mega-nerd.com/FPcast/ led me to believe it's safe
> to assume it always rounds towards the nearest whole number.  Are
> there platforms where that's not the case?

We prefer using ints here.

>>>>> +            assert(nbits < ctx->cid_table->bit_depth + 4);
>>>> 
>>>> assert would kill the program.
>>> That would indicate a serious bug.  The assert merely checks we
>>> are not
>> reading past the end of a table.
>>> BTW, I've come by the following comment in mpegvideo_enc.c: //non
>>> c quantize code returns incorrect block_last_index FIXME If still
>>> true,
>> that could trigger this assert.
>>> 
>> 
>> It's a problem, but can this actually happen ?
> 
> It happened to me once while I was in the middle of implementing
> 10bit support, which made me introduce that assert.  It hasn't
> happened since.  I hope it was because I did something wrong back
> then.

Please check.

>>>> [...]
>>>> 
>>>> dct_quantize_mmx/sse2/etc .. is used if present, see
>> QMAX_SHIFT_MMX.
>>> I specifically request FF_DCT_10BIT, so I won't get any of
>>> those.
>> 
>> Ultimately you can only replace the dct in dct_quantize.
> 
> There are also assembly versions of the whole dct_quantize function, 
> but they check for dct_algo as well, so requesting FF_DCT_10BIT would
> disable them: ---------------------------------------- 
> if(dct_algo==FF_DCT_AUTO || dct_algo==FF_DCT_MMX){ #if HAVE_SSSE3 
> if(mm_flags & AV_CPU_FLAG_SSSE3){ s->dct_quantize=
> dct_quantize_SSSE3; } else #endif if(mm_flags & AV_CPU_FLAG_SSE2){ 
> s->dct_quantize= dct_quantize_SSE2; } else if(mm_flags &
> AV_CPU_FLAG_MMX2){ s->dct_quantize= dct_quantize_MMX2; } else { 
> s->dct_quantize= dct_quantize_MMX; } } 
> ----------------------------------------
> 
>> Besides, this modification makes a change that is specific to dnxhd
>> in generic mpeg code, which I'm not a big fan of.
> 
> That's not quite the case.

It is, it's modifying dct_quantize_c for 10bit dct only which is
specific to dnxhd, it's quite hackish IMHO.

>> [...]
>> 
>>>> I think it's time to separate the dnxhd quantization fonction
>>>> from the mpeg one and change QMAT_SHIFT only there. Remove
>>>> MpegEncContext from DNxHDEncContext, add
>> dnxhd_template_mmx.c
>>>> in x86 and clean it up (intra only, out format, aic only
>>>> etc..)
>>> I believe we only use dct_quantize() from MpegEncContext, and
>>> there
>> exists quite a few assembly versions of that.  I don't think it's a
>> good idea to duplicate that code.
>> 
>> The code has overflow problems already due to the shift.
> 
> For 8 bit DNxHD it doesn't.

I think it does. The <<4 makes it overflow. In convert matrix, instead
of 1 change it to 4, and you will see the warning.

> Moving away from MpegEncContext would hurt 8 bit DNxHD performance. 

It will actually make it faster by removing a few ifs.

> I personally don't care about performance much, so if concensus is to
> go that way, I would be happy to do it.

Yes.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org