[gst-devel] [gst-cvs] wtay gst-plugins-good: gst-plugins-good/gst-plugins-good/gst/rtp/

[Wai-Ming Ho]

Hi,

On Wed, 2007-12-12 at 10:28 +0100, Peter Kjellerstedt wrote:

> > Patch by: Wai-Ming Ho <webregbox at yahoo dot co dot uk>
> > * gst/rtp/gstrtph264pay.c: (gst_rtp_h264_pay_init),
> > (gst_rtp_h264_pay_finalize), (gst_rtp_h264_pay_setcaps),
> > (next_start_code), (is_nal_equal), (gst_rtp_h264_pay_decode_nal),
> > (encode_base64), (gst_rtp_h264_pay_parse_sps_pps),
> > (gst_rtp_h264_pay_handle_buffer):
> > * gst/rtp/gstrtph264pay.h:
> > Use higher performance start-code searching.
> > Parse NALs and store SPS, PPS and profile in the caps so that they can
> > be used in the SDP. Fixes #502814.
> > 
> > Modified files:
> >     .               : ChangeLog
> >     gst/rtp         : gstrtph264pay.c gstrtph264pay.h
> > 
> > Links:
> >
> http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-good/Ch
> angeLog.diff?r1=1.3212&r2=1.3213
> >
> http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-good/gs
> t/rtp/gstrtph264pay.c.diff?r1=1.3&r2=1.4
> >
> http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-good/gs
> t/rtp/gstrtph264pay.h.diff?r1=1.1&r2=1.2
> 
> * In gst_rtp_h264_pay_finalize(): 
>   Why check for NULL before calling g_free()?
> * In encode_base64():
>   Why implement yet another base64-encoder? Now there is one
>   in the RTSP-code and one here. Wouldn't it make sense to have
>   one implementation for all of GStreamer? And if glib >= 2.12
>   then I guess you could use g_base64_encode(), and not need
>   to have any at all.

Pls check with Wim Taymans. The original patch proposed on Bugzilla uses
g_base64_encode().

http://bugzilla.gnome.org/show_bug.cgi?id=502814

> * Why whould is_nal_equal() be faster than memcmp()?
>   On what architectures?
>   Oh, and the comment preceding it mentions memcpy rather 
>   than memcmp()...

fingers typing error :-) it's suppose to read memcmp() 

I tested it on Ubuntu 7.04 /Intel Genuine Intel(R) CPU 2140  @ 1.60GHz
stepping 02. when I proposed the patch, is_nal_equal() is either the
patched implementation or a simple call to memcmp().

As for why, I would have to look at the gcc generated code but a quick
guess would be the 32-bit XOR's instead of byte-wise CMP (since memcmp()
needs to return either equal, less or greater). 

Other than that, if the code is not critical performance hit, I would be
pretty happy if it was a simple call to memcmp(). The gain of 20% is
observed only when both blocks are identical most (or all in the case os
single SPS/PPS H264 bitstreams) of the time.

> * Why not make gst_rtp_h264_pay_decode_nal() return a gboolean
>   denoting the updated status, rather than making it take a
>   gboolean return parameter?

That would be a good idea. 

> 
> //Peter
> 
> 
> -------------------------------------------------------------------------
> SF.Net email is sponsored by:
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://sourceforge.net/services/buy/index.php
> _______________________________________________
> gstreamer-devel mailing list
> gstreamer-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel