[vlc-devel] Patch to add support for HTTP Live Streaming w/requested changes
Keary Griffin
keary.griffin at unwiredappeal.com
Tue Mar 30 04:58:15 CEST 2010
I'm in the process of creating the new patches that incorporates the
fixes/suggestions mentioned here. I was in the middle of looking into
what it would take to use dprintf as suggested, but am pretty sure I'm
not familiar enough with automake/autoconf to do it with any degree of
confidence, and would rather not do it badly.
Having said that it occurred to me that simply using vlc_fopen & fprintf
would make even more sense, seeing as it's a text file that I am
writing, and am calling write in a loop, writing small sections at a time.
Make sense?
Keary
Rémi Denis-Courmont wrote:
> Hello,
>
> Is there any reason why you don't use git format? It makes merging more
> difficult for us. Comments follow.
>
>
> (...)
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_sout.h>
> +#include <vlc_block.h>
> +#include <vlc_fs.h>f
>
> Typo................^
>
> +#include <vlc_strings.h>
> +#include <vlc_charset.h>
> +#if defined( WIN32 ) && !defined( UNDER_CE )
> +# include <io.h>
> +# define lseek _lseeki64
>
> This is not needed, is it?
>
> +struct sout_access_out_sys_t
> +{
> + char *psz_cursegPath;
> + char *psz_indexPath;
> + char *psz_indexUrl;
> + mtime_t i_opendts;
> + mtime_t i_seglenm;
> + int i_handle;
> + int i_segment;
>
> unsigned
>
> + int i_seglen;
>
> size_t
>
> + int i_numsegs;
>
> unsigned
>
> + bool b_delsegs;
> + bool b_ratecontrol;
> + bool b_splitanywhere;
> +};
> +
> +/*****************************************************************************
> + * Open: open the file
> +
> *****************************************************************************/
> +static int Open( vlc_object_t *p_this )
> +{
> + sout_access_out_t *p_access = (sout_access_out_t*)p_this;
> + sout_access_out_sys_t *p_sys;
> + char *psz_idx;
> +
> + config_ChainParse( p_access, SOUT_CFG_PREFIX, ppsz_sout_options,
> p_access->p_cfg );
> +
> + if( !p_access->psz_path )
> + {
> + msg_Err( p_access, "no file name specified" );
> + return VLC_EGENERIC;
> + }
> +
> + p_access->pf_write = Write;
> + p_access->pf_read = Read;
> + p_access->pf_seek = Seek;
> + p_access->pf_control = Control;
> + if( !( p_sys = malloc ( sizeof( *p_sys ) ) ) )
> + return VLC_ENOMEM;
> +
> + p_sys->i_seglen = var_GetInteger( p_access, SOUT_CFG_PREFIX "seglen" );
> + p_sys->i_seglenm = UINT64_C(1000000) * p_sys->i_seglen;
>
> Use CLOCK_FREQ.
>
> + p_sys->psz_indexPath = 0;
>
> Use NULL.
>
> + p_sys->psz_indexUrl = var_GetNonEmptyString( p_access, SOUT_CFG_PREFIX
> "index-url" );
> +
> + psz_idx = var_GetNonEmptyString( p_access, SOUT_CFG_PREFIX "index" );
> + if ( psz_idx )
> + {
> + char *psz_tmp;
> + if ( ! ( psz_tmp = str_format( p_access, psz_idx ) ) )
> + {
> + free( p_sys );
> + return VLC_ENOMEM;
>
> p_sys->psz_indexUrl is leaked here, no?
>
> + }
> + path_sanitize( psz_tmp );
> + p_sys->psz_indexPath = psz_tmp;
> + vlc_unlink( p_sys->psz_indexPath );
> + }
> +
> + p_access->p_sys = p_sys;
> + p_sys->i_handle = -1;
> + p_sys->i_segment = 0;
> + p_sys->psz_cursegPath = 0;
>
> NULL.
>
> +
> + return VLC_SUCCESS;
> +}
> +
> (...)
> +static char *formatSegmentPath( sout_access_out_t *p_access, char *psz_path,
> int i_seg, bool b_sanitize )
> +{
> + char *psz_tmp;
> + char *p1, *p2;
> +
> + if ( ! ( psz_tmp = str_format( p_access, psz_path ) ) )
> + return NULL;
> +
> + p1 = psz_tmp;
> + while( *p1 && *p1 != '#' )
> + p1++;
>
> Use strspn().
>
> + if ( *p1 ) {
> + p2 = p1+1;
> + while( *p2 && *p2 == '#' )
> + p2++;
>
> Use strspn().
>
> + int numcnt = p2 - p1;
> + char *segstr;
> + if ( asprintf( &segstr, "%0*d", numcnt, i_seg ) < 0 )
> + {
> + free( psz_tmp );
> + return NULL;
> + }
> + char *psz_tmp2;
> + *p1 = '\0';
> + int ret = asprintf( &psz_tmp2, "%s%s%s", psz_tmp, segstr, p2 );
> + free( psz_tmp );
>
> Could this not be done with a single asprintf() call?
>
>
> + const char *lo = ToLocale ( oldpath );
> + if (lo == NULL)
> + goto error;
> +
> + const char *ln = ToLocale ( newpath );
> + if (ln == NULL)
> + {
> + LocaleFree ( lo );
> +error:
> + errno = ENOENT;
> + return -1;
> + }
> +
> + int ret = rename ( lo, ln );
> + LocaleFree ( lo );
> + LocaleFree ( ln );
> + return ret;
>
> Couldn't this use vlc_rename() in the non Windows case. Just make a separate
> patch to export it.
>
> (...)
> + strcpy( psz_idxTmp, p_sys->psz_indexPath );
> + strcpy( psz_idxTmp + strlen( psz_idxTmp ), TMP_IDX_SUFFIX );
> + vlc_unlink( psz_idxTmp );
>
> Is this needed?
>
> + fd = vlc_open( psz_idxTmp, O_RDWR | O_CREAT | O_LARGEFILE |
> + O_TRUNC, 0666 );
>
> s/O_RDWR/O_WRONLY/
>
> + if ( fd == -1 )
> + {
> + msg_Err( p_access, "cannot open index file `%s' (%m)", psz_idxTmp
> );
> + free( psz_idxTmp );
> + return -1;
> + }
> + if ( asprintf( &psz_buf, "#EXTM3U\n#EXT-X-TARGETDURATION:%d\n#EXT-X-
> MEDIA-SEQUENCE:%d\n", p_sys->i_seglen, i_firstseg ) < 0 )
> + {
> + free( psz_idxTmp );
> + close( fd );
> + return -1;
> + }
> +
> + val = write ( fd, psz_buf, strlen( psz_buf ) );
>
> You could avoid strlen() using the result of asprintf().
> Arguably dprintf() should be used (it might need to be added to compat/*).
>
> Same later.
>
> + if ( asprintf( &psz_buf, "#EXTINF:%d\n%s", p_sys->i_seglen,
> psz_name ) < 0)
> + {
> + free( psz_name ) ;
> + free( psz_idxTmp );
> + close( fd );
> + return -1;
> + }
> +
> + val = write( fd, psz_buf, strlen( psz_buf ) );
> + if ( val >= 0 )
> + val = write( fd, "\n", strlen( "\n" ) );
>
> Just write the newline in the asprintf(). This saves one system call.
>
> + /* Only need to repeat this on Win32 which doesn't support
> + atomic renames */
> +#ifdef WIN32
> + int i_attempt = 1;
> + do
> + {
> +#endif
> + val = prv_vlc_rename( psz_idxTmp, p_sys->psz_indexPath );
> +#ifdef WIN32
> + if ( val < 0 && i_attempt < MAX_RENAME_RETRIES)
> + {
> + msg_Warn ( p_access, "Rename failed -- Retrying");
> + msleep(1000*100);
> + }
> + }
> + while(val < 0 && i_attempt++ < MAX_RENAME_RETRIES);
> +#endif
>
> I would rather put this ugliness in the prv_vlc_rename() function...
>
> (...)
> +static void Close( vlc_object_t * p_this )
> +{
> + sout_access_out_t *p_access = (sout_access_out_t*)p_this;
> + sout_access_out_sys_t *p_sys = p_access->p_sys;
> +
> +
> + closeCurrentSegment( p_access, p_sys, true );
> + if ( p_sys->psz_indexPath )
> + free( p_sys->psz_indexPath );
>
> The if statement is not needed here.
>
> (...)
> +static ssize_t openNextFile( sout_access_out_t *p_access,
> sout_access_out_sys_t *p_sys )
> +{
> + int fd;
> +
> + int i_newseg = p_sys->i_segment + 1;
> +
> + char *psz_seg = formatSegmentPath( p_access, p_access->psz_path,
> i_newseg, true );
> + if ( !psz_seg )
> + return -1;
> +
> + fd = vlc_open( psz_seg, O_RDWR | O_CREAT | O_LARGEFILE |
> + O_TRUNC, 0666 );
>
> O_WRONLY.
>
> I guess this should be a separate patch:
>
> diff --git a/modules/mux/mpeg/ts.c b/modules/mux/mpeg/ts.c
> index 34e4e3e..893140e 100644
> --- a/modules/mux/mpeg/ts.c
> +++ b/modules/mux/mpeg/ts.c
> @@ -2019,6 +2019,12 @@ static block_t *TSNew( sout_mux_t *p_mux, ts_stream_t
> *p_stream,
> }
>
> p_ts = block_New( p_mux, 188 );
> +
> + if (b_new_pes && !(p_pes->i_flags & BLOCK_FLAG_NO_KEYFRAME) && p_pes-
>
>> i_flags & BLOCK_FLAG_TYPE_I)
>>
> + {
> + p_ts->i_flags |= BLOCK_FLAG_TYPE_I;
> + }
> +
> p_ts->i_dts = p_pes->i_dts;
>
> p_ts->p_buffer[0] = 0x47;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20100329/902e7276/attachment.html>
More information about the vlc-devel
mailing list