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

[Julien 'Lta' BALLET]

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:
> <lawyer mode>
> Shouldn't it be H.264/H.262 or MPEG4-Part10/MPEG2 ?
> </lawyer>

I've added both to satisfy everybody.

>
> This could be a char [][12] or char [][9] if you reduce "unspecified"
> length.
>

What would you suggest ? "unspec", "notset" ?

>
> 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 :)

> > +    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.

> > +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.

>
> 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);

> > +        goto error;
>
> enc->p_sys is not assigned yet here

[...]

> > +    if (likely(sys)) {
>
> Can !sys be true ?

It can on the case right above.

> > +    // 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.

> > +    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 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.



Regards,
Julien 'Lta' BALLET.