[vlc-devel] [PATCH] Adds Support for Intel's QuickSync Video encoder

[Rafaël Carré]

Le 10/05/2013 19:10, Julien 'Lta' BALLET a écrit :
> Hi,
> 
> Thank you for the extensive (quality) review. I've integrated most of suggested
> changes in the patch, but there's stll a few points on which i've questions
> before i'm able to provide a new patchset.
> 
> 
> On Fri, May 10, 2013 at 4:33 AM, Rafaël Carré <funman at videolan.org> wrote:
>>
>> This could be a char [][12] or char [][9] if you reduce "unspecified"
>> length.
>>
> 
> What would you suggest ? "unspec", "notset" ?

dunno

>> char [][5] would take 4 * 5 = 20 bytes instead of 4 * 4 + 4 + 4 + 3 + 5
>> = 32 on 32 bits or 8 * 4 + 4 + 4 + 3 + 5 = 48 on 64bits.
>>
>> It is not very important and can be done later if you wish but I guessed
>> I would let you know.
>>
> 
> Thank you. I haven't been programming in c for a while, so i guess i
> forgot about
> how fun it was :)

Well C gives you potential for optimization unlike other languages so
feel free to use it or not.

RAM is cheap these days, so what this does is mostly less pointer
relocations, so less work for the linker.

If you have a big string that just means all the entries in the array
must have the same size so bytes are wasted.

>>> +    set_capability("encoder", 0)
>>
>> Is the encoder admitedly worse than x264 ?
>>
> 
> I simply decided i wasn't the right person to choose what priority to
> a new codec. To answer ileoo's question
> at the same time, my initial idea was to give this codec a higher
> priority than x264's one but to disable
> software mode by default. That way we use the underlying hardware when
> available and revert to
> our good ol' x264 if not.

I would guess that x264 should always be the default because afaiu it
gives the bestest quality in the world.

>>> +static inline mtime_t qsv_timestamp_to_mtime(uint64_t mfx_ts)
>>> +{
>>> +    return mfx_ts / INT64_C(9) * INT64_C(100);
>>
>> Are overflow and unsigned -> signed conversion handled?
>>
> 
> I would really appreciate if you could me some advices on how to
> handle that correctly. If this ever happens
> wouldn't it anyway lead to a timestamp discontinuity ? I was initialy
> casting to double to avoid this, but it
> seemed the rest of vlc's code was avoid such thing.

Well what about starting the pts you get from vlc from 0 ?

Just remove initial offset so vlc pts would overflow before qsv pts anyway.

>> old_pic = .....
>>
>>> +            old_pic = (picture_t *)frame->Data.MemId;
>>> +            if (old_pic)
>>> +                picture_Release(old_pic);
> 
> Not sure of what you meant there, i replaced that with :
> 
>         if (frame->Data.MemId)
>             picture_Release((picture_t *)frame->Data.MemId);

What I wrote was:

-----------------------
if (frame->Data.Locked)
    continue;

old_pic = .....
-----------------------

-> one less level of indentation, easier to read

>>> +        goto error;
>>
>> enc->p_sys is not assigned yet here
> 
> [...]
> 
>>> +    if (likely(sys)) {
>>
>> Can !sys be true ?
> 
> It can on the case right above.

But then you are leaking ?

>>> +    // Allocate block_t and prepare mfxBitstream for encoder
>>> +    if (!(task->block = block_Alloc(sys->params.mfx.BufferSizeInKB * 1000))) {
>>
>> Not 1024 ?
> 
> For some reason ... no. Ask Intel guys if some insights are needed.

OK

>>> +    sts = MFXVideoENCODE_EncodeFrameAsync(sys->session, 0, frame, &task->bs, &task->syncp);
>>> +    while (sts == MFX_WRN_DEVICE_BUSY) {
>>> +        msg_Info(enc, "Device is busy, let's wait and retry");
>>> +        msleep(QSV_SPINLOCK_TIME);
>>
>> Hum this is not a spinlock ?
> 
> Yeah, i know, but 'spinlock' is much cooler word than 'busywait' and i
> always wanted to write that at least once :)
> I've changed it in the code. Sorry :D
> 
>>
>> Can't you hold data until pf_encode is called again?
> 
> I think i could but i'm not sure this is actually a good idea. The
> probleme here, afaik, is that we
> are feeding the encoder too fast. Queuing the frames then might lead
> to an increasing memory
> consumption over time which can become a problem for long streams.

The problem is with unpaced inputs like file?

Isn't there a way to be notified when the encoder is not busy anymore?

> The frame/task management code is already very messy, despite all my
> efforts to hide Intel's API
> complexity. Imho, adding frame queuing code here will reach a certain
> thresold in code (un)readability.
> Btw, Intel Media SDK is already buffering frames internally, in a
> configurable way (--sout-qsv-async-depth=x),
> i believe queuing frames won't then lead to any speed improvement.
> 
> QSV_{SPINLOCK|BUSYWAY}_TIME value, on the other hand, has been defined
> pretty arbitrarily.
> Intel's using 1ms in all their examples, but it seemed rather short to
> me, especially regarding windows
> timer granularity.