[vlc-devel] [PATCH 1/1] codec: Add GStreamer based video decoder module

[Rafaël Carré]

On 04/19/14 13:16, ved kpl wrote:
> Thanks for the comments Rafael..
> Yeah, GStreamer should have VLC based plugin :).
> 
> Will incorporate the changes as per your review.
> Had some queries though. I have put them inline.

OK, answers below

> On Sat, Apr 19, 2014 at 2:38 PM, Rafaël Carré <funman at videolan.org> wrote:

>>> +    switch( i_fmt )
>>> +    {
>>> +        case VLC_CODEC_H264:
>>
>> nitpicking: align case on the switch level
>>
>     vikram:  you mean something like:
>                           switch (a)
>                           {
>                           case 1:
>                           break;
>                           case 2:
>                           break;
>                           }

I use:

switch(a) {
case 1:
    foo();
    break;
case 2:
    break;
}

>>> +    if( unlikely( p_dec->p_sys->b_out_fmt_set == false ) )
>>> +    {
>>> +        do
>>> +        {
>>> +            if( gst_pad_has_current_caps( p_pad ) )
>>
>> while (gst_pad_has_current_caps(p_pad))

> Vikram: why "while"? Probably I will need an "if" there?

Well I assume you want to use break to exit the block, thus the
do { ... } while(0);

If you don't use break, just use if() and deindent that whole block of code
to the left.


>>> +                            b_ret = gst_structure_get_int ( p_str,
>> "width",
>>> +                                    ( gint*
>> )&p_dec->fmt_out.video.i_width );
>>> +                            b_ret &= gst_structure_get_int ( p_str,
>> "height",
>>> +                                    ( gint*
>> )&p_dec->fmt_out.video.i_height );
>>
>> I'd prefer to use && here
>> Vikram: I didn't get you. where exactly?

b_ret = gst_structure_get_int(...) && gst_structure_get_int(...);

You could also use video_format_t *fmt = &p_dec->fmt_out.video;


>>> +/* Copy the frame data from the GstBuffer (from decoder)
>>> + * to the picture obtained from downstream in VLC */
>>> +static void gst_CopyPicture( picture_t *p_pic, GstVideoFrame *frame )
>>
>> So it's not possible to tell Gst where to decode data and let it do
>> the memcpy if e.g. using a hardware decoder?
>>
>    Vikram: Yes, it is possible and I am working on it as time permits.  As
> I have mentioned in the cover letter, zero-copy is possible.
>                That'll  will require some proper  handling w.r.t buffer
> management. I plan to add this part as a next patch.

Good, let's keep that for next patch then.

>>> +#define VLC_GST_CHECK(r, v, s, t, x) \
>>> +    { if(r == v) { msg_Info(p_dec, s); i_rval = t; goto x; } }
>>
>> msg_Err ?
>> Vikram: Yeah, I had this as msg_Err earlier, But I don't know prompted me
>> to use Info :-/. I will revert it back to Err.

Maybe msg_Info() is more visible in the log ;)


>>> +/* Decode */
>>> +static picture_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )
>>> +{
>>> +    block_t *p_block = NULL;
>>> +    picture_t *p_pic = NULL;
>>> +    decoder_sys_t *p_sys = p_dec->p_sys;
>>> +    GstMessage* p_msg;
>>> +    gboolean b_ret;
>>> +    guint8* p_data;
>>> +    GstBuffer* p_buf;
>>> +
>>> +    if( !pp_block || !*pp_block ) {
>>> +        return NULL;
>>> +    }
>>
>> Decoder should be flushed if *pp_block == NULL
>> Vikram: I see..  So does that mean that already decoded buffers (waiting
>> in the queue to be popped out ) should also be flushed and not be rendered?

Not exactly, they should be returned by DecodeBlock

block_t *p_block = *pp_block;

if (!p_block) {
    return buffered_frame;
}

That's the case if the first calls to DecodeBlock did not return data.

See CODEC_CAP_DELAY in avcodec/video.c


>>> +    memcpy( p_data, p_block->p_buffer, p_block->i_buffer );
>>> +    p_buf = gst_buffer_new_wrapped( p_data, p_block->i_buffer );
>>
>> gst_buffer_new_wrapped_full instead of malloc+memcpy ?
>>
>    Vikram: Is it ok to call block_Release() from any thread context?  I
> thought it can only be called from DecodeBlock()  thread context.
> Neverthless, you are right. mempcy for
>                input buffer can be avoided.

Yes it is ok.

> Then you can call block_Release in the destructor
>> Vikram: If it is necessary to call block_release() only from
>> decode_block() thread context, I'll use a queue so that in destructor ,
>> instead of doing a release, I will push the block
>>
>                 to  the queue, and in decodeblock(), I'll pop the blocks
> from the queue and release them.
> 
> 
>>> +    if( unlikely( p_buf == NULL ) )
>>> +    {
>>> +        free( p_data );
>>> +        block_Release( p_block );
>>> +        msg_Err( p_dec, "failed to create gstbuffer" );
>>> +        p_dec->b_error = true;
>>> +        goto done;
>>> +    }
>>> +
>>> +    if( p_block->i_dts > VLC_TS_INVALID )
>>> +        GST_BUFFER_DTS( p_buf ) = gst_util_uint64_scale( p_block->i_dts,
>>> +                GST_SECOND, GST_MSECOND );
>>> +
>>> +    if( p_block->i_pts <= VLC_TS_INVALID )
>>> +        GST_BUFFER_PTS( p_buf ) = GST_BUFFER_DTS( p_buf );
>>> +    else
>>> +        GST_BUFFER_PTS( p_buf ) = gst_util_uint64_scale( p_block->i_pts,
>>> +                GST_SECOND, GST_MSECOND );
>>> +
>>> +    if( p_dec->fmt_in.video.i_frame_rate  &&
>>> +            p_dec->fmt_in.video.i_frame_rate_base )
>>> +        GST_BUFFER_DURATION( p_buf ) = gst_util_uint64_scale(
>> GST_SECOND,
>>> +                p_dec->fmt_in.video.i_frame_rate_base,
>>> +                p_dec->fmt_in.video.i_frame_rate );
>>
>> p_block->i_length ?
>> Vikram: oh! missed that!  i_length means duration, right?

Yes, in microsecond units (1/CLOCK_FREQ)

BTW don't hesitate to ask on IRC