[vlc-devel] [PATCH 1/1] codec: Add GStreamer based video decoder module
ved kpl
ved.kpl at gmail.com
Mon Apr 21 08:16:26 CEST 2014
Thanks Rafael, Comments in-line.
On Mon, Apr 21, 2014 at 1:07 AM, Rafaël Carré <funman at videolan.org> wrote:
> 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;
> }
>
Vikram: Agreed.
>
> >>> + 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.
> Vikram: Ok, I'll remove the dowhile(0) loop.
>
> >>> + 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(...);
> Vikram: Agreed.
>
> You could also use video_format_t *fmt = &p_dec->fmt_out.video;
> Vikram: Agreed.
>
> >>> +/* 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.
> Vikram: Thanks!
>
> >>> +#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 ;)
> Vikram: Possibly yeah! :-) Anyway, I've reverted it back to msg_Er()..
>
> >>> +/* 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
> Vikram: I see. But in GStreamer, AFAICT, I'm not sure if there is such
> event that can be passed. I will look again though. But if really there
> isn't such handling, best would be to skip to looking for messages & then
> peeking for available buffers in the output queue when p_block == NULL, in
> DecodeBlock().
>
> >>> + 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.
>
Vikram: Ok, will do the change.
>
> > 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)
> Vikram: Agreed.
>
> BTW don't hesitate to ask on IRC
>
Vikram: Yeah, Sure :)
Thanks!
> Vikram
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140421/6d956d76/attachment.html>
More information about the vlc-devel
mailing list