<div dir="ltr">Thanks Rafael, Comments in-line.<br><div class="gmail_extra"><br>On Mon, Apr 21, 2014 at 1:07 AM, Rafaël Carré <span dir="ltr"><<a href="mailto:funman@videolan.org" target="_blank">funman@videolan.org</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 04/19/14 13:16, ved kpl wrote:<br>
> Thanks for the comments Rafael..<br>
> Yeah, GStreamer should have VLC based plugin :).<br>
><br>
> Will incorporate the changes as per your review.<br>
> Had some queries though. I have put them inline.<br>
<br>
</div>OK, answers below<br>
<div class=""><br>
> On Sat, Apr 19, 2014 at 2:38 PM, Rafaël Carré <<a href="mailto:funman@videolan.org">funman@videolan.org</a>> wrote:<br>
<br>
</div><div class="">>>> + switch( i_fmt )<br>
>>> + {<br>
>>> + case VLC_CODEC_H264:<br>
>><br>
>> nitpicking: align case on the switch level<br>
>><br>
> vikram: you mean something like:<br>
> switch (a)<br>
> {<br>
> case 1:<br>
> break;<br>
> case 2:<br>
> break;<br>
> }<br>
<br>
</div>I use:<br>
<br>
switch(a) {<br>
case 1:<br>
foo();<br>
break;<br>
case 2:<br>
break;<br>
}<br></blockquote><div> Vikram: Agreed. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<div class=""><br>
>>> + if( unlikely( p_dec->p_sys->b_out_fmt_set == false ) )<br>
>>> + {<br>
>>> + do<br>
>>> + {<br>
>>> + if( gst_pad_has_current_caps( p_pad ) )<br>
>><br>
>> while (gst_pad_has_current_caps(p_pad))<br>
<br>
> Vikram: why "while"? Probably I will need an "if" there?<br>
<br>
</div>Well I assume you want to use break to exit the block, thus the<br>
do { ... } while(0);<br>
<br>
If you don't use break, just use if() and deindent that whole block of code<br>
to the left.<br>
<div class="">Vikram: Ok, I'll remove the dowhile(0) loop.<br></div><div class="">
<br>
>>> + b_ret = gst_structure_get_int ( p_str,<br>
>> "width",<br>
>>> + ( gint*<br>
>> )&p_dec->fmt_out.video.i_width );<br>
>>> + b_ret &= gst_structure_get_int ( p_str,<br>
>> "height",<br>
>>> + ( gint*<br>
>> )&p_dec->fmt_out.video.i_height );<br>
>><br>
>> I'd prefer to use && here<br>
>> Vikram: I didn't get you. where exactly?<br>
<br>
</div>b_ret = gst_structure_get_int(...) && gst_structure_get_int(...);<br></div>Vikram: Agreed.
<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
You could also use video_format_t *fmt = &p_dec->fmt_out.video;<br>
<div class="">Vikram: Agreed.<br></div><div class="">
<br>
>>> +/* Copy the frame data from the GstBuffer (from decoder)<br>
>>> + * to the picture obtained from downstream in VLC */<br>
>>> +static void gst_CopyPicture( picture_t *p_pic, GstVideoFrame *frame )<br>
>><br>
>> So it's not possible to tell Gst where to decode data and let it do<br>
>> the memcpy if e.g. using a hardware decoder?<br>
>><br>
> Vikram: Yes, it is possible and I am working on it as time permits. As<br>
> I have mentioned in the cover letter, zero-copy is possible.<br>
> That'll will require some proper handling w.r.t buffer<br>
> management. I plan to add this part as a next patch.<br>
<br>
</div>Good, let's keep that for next patch then.<br>
<div class="">Vikram: Thanks!<br></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="">
>>> +#define VLC_GST_CHECK(r, v, s, t, x) \<br>
>>> + { if(r == v) { msg_Info(p_dec, s); i_rval = t; goto x; } }<br>
>><br>
>> msg_Err ?<br>
>> Vikram: Yeah, I had this as msg_Err earlier, But I don't know prompted me<br>
>> to use Info :-/. I will revert it back to Err.<br>
<br>
</div>Maybe msg_Info() is more visible in the log ;)<br>
<div class="">Vikram: Possibly yeah! :-) Anyway, I've reverted it back to msg_Er()..<br></div><div class="">
<br>
>>> +/* Decode */<br>
>>> +static picture_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )<br>
>>> +{<br>
>>> + block_t *p_block = NULL;<br>
>>> + picture_t *p_pic = NULL;<br>
>>> + decoder_sys_t *p_sys = p_dec->p_sys;<br>
>>> + GstMessage* p_msg;<br>
>>> + gboolean b_ret;<br>
>>> + guint8* p_data;<br>
>>> + GstBuffer* p_buf;<br>
>>> +<br>
>>> + if( !pp_block || !*pp_block ) {<br>
>>> + return NULL;<br>
>>> + }<br>
>><br>
>> Decoder should be flushed if *pp_block == NULL<br>
>> Vikram: I see.. So does that mean that already decoded buffers (waiting<br>
>> in the queue to be popped out ) should also be flushed and not be rendered?<br>
<br>
</div>Not exactly, they should be returned by DecodeBlock<br>
<br>
block_t *p_block = *pp_block;<br>
<br>
if (!p_block) {<br>
return buffered_frame;<br>
}<br>
<br>
That's the case if the first calls to DecodeBlock did not return data.<br>
<br>
See CODEC_CAP_DELAY in avcodec/video.c<br>
<div class="">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().<br>
</div><div class="">
<br>
>>> + memcpy( p_data, p_block->p_buffer, p_block->i_buffer );<br>
>>> + p_buf = gst_buffer_new_wrapped( p_data, p_block->i_buffer );<br>
>><br>
>> gst_buffer_new_wrapped_full instead of malloc+memcpy ?<br>
>><br>
> Vikram: Is it ok to call block_Release() from any thread context? I<br>
> thought it can only be called from DecodeBlock() thread context.<br>
> Neverthless, you are right. mempcy for<br>
> input buffer can be avoided.<br>
<br>
</div>Yes it is ok.<br></blockquote><div> Vikram: Ok, will do the change. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<div><div class="h5"><br>
> Then you can call block_Release in the destructor<br>
>> Vikram: If it is necessary to call block_release() only from<br>
>> decode_block() thread context, I'll use a queue so that in destructor ,<br>
>> instead of doing a release, I will push the block<br>
>><br>
> to the queue, and in decodeblock(), I'll pop the blocks<br>
> from the queue and release them.<br>
><br>
><br>
>>> + if( unlikely( p_buf == NULL ) )<br>
>>> + {<br>
>>> + free( p_data );<br>
>>> + block_Release( p_block );<br>
>>> + msg_Err( p_dec, "failed to create gstbuffer" );<br>
>>> + p_dec->b_error = true;<br>
>>> + goto done;<br>
>>> + }<br>
>>> +<br>
>>> + if( p_block->i_dts > VLC_TS_INVALID )<br>
>>> + GST_BUFFER_DTS( p_buf ) = gst_util_uint64_scale( p_block->i_dts,<br>
>>> + GST_SECOND, GST_MSECOND );<br>
>>> +<br>
>>> + if( p_block->i_pts <= VLC_TS_INVALID )<br>
>>> + GST_BUFFER_PTS( p_buf ) = GST_BUFFER_DTS( p_buf );<br>
>>> + else<br>
>>> + GST_BUFFER_PTS( p_buf ) = gst_util_uint64_scale( p_block->i_pts,<br>
>>> + GST_SECOND, GST_MSECOND );<br>
>>> +<br>
>>> + if( p_dec->fmt_in.video.i_frame_rate &&<br>
>>> + p_dec->fmt_in.video.i_frame_rate_base )<br>
>>> + GST_BUFFER_DURATION( p_buf ) = gst_util_uint64_scale(<br>
>> GST_SECOND,<br>
>>> + p_dec->fmt_in.video.i_frame_rate_base,<br>
>>> + p_dec->fmt_in.video.i_frame_rate );<br>
>><br>
>> p_block->i_length ?<br>
>> Vikram: oh! missed that! i_length means duration, right?<br>
<br>
</div></div>Yes, in microsecond units (1/CLOCK_FREQ)<br>
</div>Vikram: Agreed.<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
BTW don't hesitate to ask on IRC<br></blockquote><div> Vikram: Yeah, Sure :)<br><br></div><div> Thanks! <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="HOEnZb"><div class="h5">Vikram</div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="HOEnZb">
<div class="h5">_______________________________________________<br></div><div class="h5">
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="https://mailman.videolan.org/listinfo/vlc-devel" target="_blank">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div></div></blockquote></div><br></div></div>