[vlc-devel] [PATCH] Added support for VDR recordings.
Laurent Aimar
fenrir at elivagar.org
Sat May 8 22:09:57 CEST 2010
HI,
Sorry for the late review.
On Sat, Apr 17, 2010 at 03:14:18PM +0200, Tobias Güntner wrote:
> +/*****************************************************************************
> + * Open: open the directory
> + *****************************************************************************/
> +static int Open( vlc_object_t *p_this )
> +{
> + access_t *p_access = (access_t*)p_this;
> + access_sys_t *p_sys;
> +
> + if( !p_access->psz_path )
> + return VLC_EGENERIC;
Useless test, psz_path cannot be NULL.
> +
> + if( !strcmp( p_access->psz_path, "-" ) )
> + return VLC_EGENERIC;
> +
> + /* Some tests can be skipped if this module was explicitly requested.
> + * That way, the user can play "corrupt" recordings if necessary
> + * and we can avoid false positives in the general case. */
> + bool b_strict = !p_access->psz_access ||
> + strcmp( p_access->psz_access, "vdr" );
Same for psz_access (cannot be NULL).
> +/*****************************************************************************
> + * Close: close the target
> + *****************************************************************************/
> +static void Close( vlc_object_t * p_this )
> +{
> + access_t *p_access = (access_t*)p_this;
> + access_sys_t *p_sys = p_access->p_sys;
> +
> + if( p_sys->fd != -1 )
> + close( p_sys->fd );
> + ARRAY_RESET( p_sys->file_sizes );
> +
> + if( p_sys->p_meta )
> + vlc_meta_Delete( p_sys->p_meta );
> +
> + vlc_input_title_Delete( p_sys->p_marks );
I think that p_marks can be NULL (in case of failure at least, as you call
Close() in Open().
> +/*****************************************************************************
> + * Read: concatenate all files
> + *****************************************************************************/
> +static ssize_t Read( access_t *p_access, uint8_t *p_buffer, size_t i_len )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
> +
> + while( p_sys->fd != -1 )
> + {
> + ssize_t i_ret = read( p_sys->fd, p_buffer, i_len );
> +
> + if( i_ret > 0 )
> + {
> + /* success */
> + p_access->info.i_pos += i_ret;
> + UpdateVideoFileSize( p_access );
> + FindSeekpoint( p_access );
> + return i_ret;
> + }
> + else if( i_ret == 0 )
> + {
> + /* check for new files in case the recording is still active */
> + if( p_sys->i_current_file >= FILE_COUNT - 1 )
> + ImportNextVideo( p_access );
> + /* play next file or stop on "real" eof */
> + if( SwitchVideoFile( p_access, p_sys->i_current_file + 1 ) )
> + continue;
> + else
> + break;
> + }
> + else if( errno == EINTR )
> + {
> + /* try again later */
> + return -1;
I haven't followed if this one was accepted or not by Rémi.
> + }
> + else
> + {
> + /* abort on read error */
> + msg_Err( p_access, "failed to read (%m)" );
> + dialog_Fatal( p_access, _("File reading failed"), "%s",
> + _("VLC could not read the file.") );
> + break;
> + }
> + }
> +
> + /* stop further reading */
> + p_access->info.b_eof = true;
> + return 0;
> +/*****************************************************************************
> + * FindSeekpoint: Changes the chapter index if necessary
> + *****************************************************************************/
> +static void FindSeekpoint( access_t *p_access )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
Is it possible to end up here with p_marks being NULL?
> + if( !p_sys->p_marks )
> + return;
> +
> + int i_new_seekpoint = p_access->info.i_seekpoint;
> + if( p_access->info.i_pos < (uint64_t)p_sys->p_marks->
> + seekpoint[ p_access->info.i_seekpoint ]->i_byte_offset )
> + {
> + /* i_pos moved backwards, start fresh */
> + i_new_seekpoint = 0;
> + }
> +
> + /* only need to check the following seekpoints */
> + while( i_new_seekpoint + 1 < p_sys->p_marks->i_seekpoint &&
> + p_access->info.i_pos >= (uint64_t)p_sys->p_marks->
> + seekpoint[ i_new_seekpoint + 1 ]->i_byte_offset )
> + {
> + i_new_seekpoint++;
> + }
I am not sure, but maybe using bsearch() would be simpler.
> +
> + /* avoid unnecessary events */
> + if( p_access->info.i_seekpoint != i_new_seekpoint )
> + {
> + p_access->info.i_seekpoint = i_new_seekpoint;
> + p_access->info.i_update |= INPUT_UPDATE_SEEKPOINT;
> + }
> +}
> +/*****************************************************************************
> + * SwitchVideoFile: Close the current file and open another
> + *****************************************************************************/
> +static bool SwitchVideoFile( access_t *p_access, unsigned i_file )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
> +
> + /* requested file already open? */
> + if( p_sys->fd != -1 && p_sys->i_current_file == i_file )
> + return true;
> +
> + /* close old file */
> + if( p_sys->fd != -1 )
> + {
> + close( p_sys->fd );
> + p_sys->fd = -1;
> + }
> +
> + /* switch */
> + if( i_file >= FILE_COUNT )
> + return false;
> + p_sys->i_current_file = i_file;
> +
> + /* open new file */
> + char *psz_path = GetFilePath( p_access, i_file );
> + if( !psz_path )
> + return false;
> + p_sys->fd = vlc_open( psz_path, O_RDONLY );
> +
> + if( p_sys->fd == -1 )
> + {
> + msg_Err( p_access, "Failed to open %s: %m", psz_path );
> + dialog_Fatal (p_access, _("File reading failed"), _("VLC could not"
> + " open the file \"%s\"."), psz_path);
> + free( psz_path );
> + return false;
> + }
> +
> + /* cannot handle anything except normal files */
> + struct stat st;
> + if( fstat( p_sys->fd, &st ) || !S_ISREG( st.st_mode ) )
> + {
> + msg_Err( p_access, "%s is not a regular file", psz_path );
> + dialog_Fatal (p_access, _("File reading failed"), _("VLC could not"
> + " open the file \"%s\"."), psz_path);
> + free( psz_path );
> + close( p_sys->fd );
> + p_sys->fd = -1;
> + return false;
> + }
I think a clean goto would reduce the duplication of code.
> +/*****************************************************************************
> + * UpdateVideoFileSize: Fix size if the (last) video file is still growing
> + *****************************************************************************/
> +static void UpdateVideoFileSize( access_t *p_access )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
> + struct stat st;
> +
> + if( p_access->info.i_size >= p_access->info.i_pos )
> + return;
It would be better to do as the file access does it (ie checking periodically).
> + /* TODO: not sure if this can happen or what to do in this case */
> + if( fstat( p_sys->fd, &st ) )
> + return;
> + if( (uint64_t)st.st_size <= CURRENT_FILE_SIZE )
> + return;
> +
> + p_access->info.i_size -= CURRENT_FILE_SIZE;
> + CURRENT_FILE_SIZE = st.st_size;
> + p_access->info.i_size += CURRENT_FILE_SIZE;
> + p_access->info.i_update |= INPUT_UPDATE_SIZE;
> +}
> +/*****************************************************************************
> + * OpenRelativeFile: Open file relative to base directory for reading.
> + *****************************************************************************/
> +static FILE *OpenRelativeFile( access_t *p_access, const char *psz_file,
> + bool b_text_mode )
> +{
> + /* build path and add extension */
> + char *psz_path;
> + if( asprintf( &psz_path, "%s" DIR_SEP "%s%s",
> + p_access->psz_path, psz_file,
> + p_access->p_sys->b_ts_format ? "" : ".vdr" ) == -1 )
> + return NULL;
> +
> + FILE *file = vlc_fopen( psz_path, b_text_mode ? "rt" : "rb" );
Are you sure using the text mode is needed ? If no, it would simplify things
a bit.
> + if( !file )
> + msg_Warn( p_access, "Failed to open %s: %m", psz_path );
> + free( psz_path );
> +
> + return file;
> +}
> +/*****************************************************************************
> + * ReadLine: Read a line of text. Returns false on error or EOF
> + *****************************************************************************/
> +static bool ReadLine( char **ppsz_line, size_t *pi_size, FILE *p_file )
> +{
> + ssize_t read = getline( ppsz_line, pi_size, p_file );
It seems that getline is a GNU extension and if so, a check in configure will be
needed.
Using stream_UrlNew+stream_ReadLine might be another way.
> + if( read == -1 )
> + {
> + /* automatically free buffer on eof */
> + free( *ppsz_line );
> + *ppsz_line = NULL;
> + return false;
> + }
> +
> + if( read > 0 && (*ppsz_line)[ read - 1 ] == '\n' )
> + (*ppsz_line)[ read - 1 ] = '\0';
Is it possible that some file are recorded using \r or \r\n, or it is always
a unique \n?
> +/*****************************************************************************
> + * ImportMeta: Import meta data
> + *****************************************************************************/
> +static void ImportMeta( access_t *p_access )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
> +
> + FILE *infofile = OpenRelativeFile( p_access, "info", true );
> + if( !infofile )
> + return;
> +
> + vlc_meta_t *p_meta = vlc_meta_New();
> + p_sys->p_meta = p_meta;
> + if( !p_meta )
> + {
> + fclose( infofile );
> + return;
> + }
> +
> + char *line = NULL;
> + size_t line_len;
> + char *psz_title = NULL, *psz_smalltext = NULL, *psz_date = NULL;
> +
> + while( ReadLine( &line, &line_len, infofile ) )
> + {
> + if( !isalpha( (unsigned char)line[0] ) || line[1] != ' ' )
isalpha takes an int, and so casting is useless.
> + continue;
> +
> + if( line[0] == 'C' )
> + {
> + char *psz_name = strchr( line + 2, ' ' );
> + if( psz_name )
> + {
> + *psz_name = '\0';
> + vlc_meta_AddExtra( p_meta, "Channel", psz_name + 1 );
> + }
> + vlc_meta_AddExtra( p_meta, "Transponder", line + 2 );
> + }
> +
Adding 'else' (here and later) would make the code easier to read.
Using a switch() is also a possibility.
> + if( line[0] == 'E' )
> + {
> + unsigned long i_id, i_start, i_length;
Using unsigned long doesn't help, either you need a 64 bits and so use int64_t
or an unsigned will be enough.
> + if( sscanf( line + 2, "%lu %lu %lu", &i_id, &i_start, &i_length ) == 3 )
> + {
> + char str[50];
> + struct tm tm;
> + time_t start = i_start;
> + localtime_r( &start, &tm );
> +
> + /* TODO: locale */
> + strftime( str, 50, "%Y-%m-%d %H:%M", &tm );
sizeof(str) instead of 50.
> + vlc_meta_AddExtra( p_meta, "Date", str );
> + free( psz_date );
> + psz_date = strdup( str );
> +
> + /* display in minutes */
> + i_length = ( i_length + 59 ) / 60;
> + sprintf( str, "%u:%02u", i_length / 60, i_length % 60 );
snprintf is better to avoid future mistake.
> + vlc_meta_AddExtra( p_meta, "Duration", str );
> + }
> + }
> +
> + if( line[0] == 'T' )
> + {
> + free( psz_title );
> + psz_title = strdup( line + 2 );
> + vlc_meta_AddExtra( p_meta, "Title", line + 2 );
> + }
> +
> + if( line[0] == 'S' )
> + {
> + free( psz_smalltext );
> + psz_smalltext = strdup( line + 2 );
> + vlc_meta_AddExtra( p_meta, "Info", line + 2 );
> + }
> +
> + if( line[0] == 'D' )
> + {
> + for( char *p = line + 2; *p; ++p )
> + {
> + if( *p == '|' )
> + *p = '\n';
> + }
> + vlc_meta_SetDescription( p_meta, line + 2 );
> + }
> +
> + /* FPS are required to convert between timestamps and frames */
> + if( line[0] == 'F' )
> + {
> + float fps = atof( line + 2 );
> + if( fps >= 1 )
> + p_sys->fps = fps;
> + vlc_meta_AddExtra( p_meta, "Frame Rate", line + 2 );
> + }
> +
> + if( line[0] == 'P' )
> + {
> + vlc_meta_AddExtra( p_meta, "Priority", line + 2 );
> + }
> +
> + if( line[0] == 'L' )
> + {
> + vlc_meta_AddExtra( p_meta, "Lifetime", line + 2 );
> + }
Using a local variable holding line + 2 would simplify the above code.
> + }
> +
> + /* create a meaningful title */
> + int i_len = 10 +
> + ( psz_title ? strlen( psz_title ) : 0 ) +
> + ( psz_smalltext ? strlen( psz_smalltext ) : 0 ) +
> + ( psz_date ? strlen( psz_date ) : 0 );
> + char *psz_display = malloc( i_len );
> + if( psz_display )
> + {
> + *psz_display = '\0';
> + if( psz_title )
> + strcat( psz_display, psz_title );
> + if( psz_title && psz_smalltext )
> + strcat( psz_display, " - " );
> + if( psz_smalltext )
> + strcat( psz_display, psz_smalltext );
> + if( ( psz_title || psz_smalltext ) && psz_date )
> + {
> + strcat( psz_display, " (" );
> + strcat( psz_display, psz_date );
> + strcat( psz_display, ")" );
> + }
> + if( *psz_display )
> + vlc_meta_SetTitle( p_meta, psz_display );
> + }
> + free( psz_display );
It might be better to move it inside the if().
> + free( psz_title );
> + free( psz_smalltext );
> + free( psz_date );
> +
> + fclose( infofile );
> +}
> +
> +/*****************************************************************************
> + * ImportMarks: Import cut marks and convert them to seekpoints
> + *****************************************************************************/
> +static void ImportMarks( access_t *p_access )
> +{
> + access_sys_t *p_sys = p_access->p_sys;
> +
> + FILE *marksfile = OpenRelativeFile( p_access, "marks", true );
> + if( !marksfile )
> + return;
> +
> + FILE *indexfile = OpenRelativeFile( p_access, "index", false );
> + if( !indexfile )
> + {
> + fclose( marksfile );
> + return;
> + }
> +
> + /* Put all cut marks in a "dummy" title */
> + input_title_t *p_marks = vlc_input_title_New();
> + if( !p_marks )
> + {
> + fclose( marksfile );
> + fclose( indexfile );
> + return;
> + }
> + p_marks->b_menu = true;
I don't think you should set b_menu. It is meant to be used when the
title is a special title used for menu (like on DVD).
> + p_marks->psz_name = strdup( _("VDR Cut Marks") );
> +
> + /* offset for chapter positions */
> + int i_chapter_offset = p_sys->fps / 1000 *
> + var_InheritInteger( p_access, "vdr-chapter-offset" );
> +
> + /* parse lines of the form "0:00:00.00 foobar" */
> + char *line = NULL;
> + size_t line_len;
> + while( ReadLine( &line, &line_len, marksfile ) )
> + {
> + int64_t i_frame = ParseFrameNumber( line, p_sys->fps );
> +
> + /* move all chapters as requested */
> + if( i_frame > -i_chapter_offset )
> + i_frame += i_chapter_offset;
> + else
> + i_frame = 0;
> +
> + uint64_t i_offset;
> + uint16_t i_file_number;
> + if( !ReadIndexRecord( indexfile, p_sys->b_ts_format,
> + i_frame, &i_offset, &i_file_number ) )
> + continue;
> + if( i_file_number < 1 || i_file_number > FILE_COUNT )
> + continue;
> +
> + /* add file sizes to get the "global" offset */
> + seekpoint_t *sp = vlc_seekpoint_New();
> + if( !sp )
> + continue;
> + sp->i_time_offset = i_frame * (int64_t)( CLOCK_FREQ / p_sys->fps );
It would probably be better to not use a cast (for precision).
> + sp->i_byte_offset = i_offset;
> + for( int i = 0; i + 1 < i_file_number; ++i )
> + sp->i_byte_offset += FILE_SIZE( i );
> + sp->psz_name = strdup( line );
> +
> + TAB_APPEND( p_marks->i_seekpoint, p_marks->seekpoint, sp );
> + }
> +
> + if( p_marks->i_seekpoint > 0 )
> + {
> + seekpoint_t *sp = vlc_seekpoint_New();
> + if( sp )
> + {
> + sp->i_byte_offset = 0;
> + sp->i_time_offset = 0;
> + sp->psz_name = strdup( _("Start") );
> + TAB_INSERT( p_marks->i_seekpoint, p_marks->seekpoint, sp, 0 );
> + }
> + }
> +
> + if( p_marks->i_seekpoint != 0 )
Merging the if() will be simpler to read.
> + p_sys->p_marks = p_marks;
> + else
> + vlc_input_title_Delete( p_marks );
> +
> + fclose( marksfile );
> + fclose( indexfile );
> +}
> +/*****************************************************************************
> + * ReadIndexRecord: lookup frame offset in index file
> + *****************************************************************************/
> +static bool ReadIndexRecord( FILE *p_file, bool b_ts, int64_t i_frame,
> + uint64_t *pi_offset, uint16_t *pi_file_num )
> +{
> + uint8_t index_record[8];
> + if( fseek( p_file, sizeof(index_record) * i_frame, SEEK_SET ) != 0 )
> + return false;
> + if( fread( &index_record, sizeof(index_record), 1, p_file ) <= 0 )
> + return false;
> +
> + /* VDR usually (only?) runs on little endian machines, but VLC has a
> + * broader audience. See recording.* in VDR source for data layout. */
> + if( b_ts )
> + {
> + uint64_t i_index_entry = GetQWLE( &index_record );
> + *pi_offset = i_index_entry & 0xFFFFFFFFFF;
You probably need INT64_C() for the constant.
> + *pi_file_num = i_index_entry >> 48;
> + }
> + else
> + {
> + *pi_offset = GetDWLE( &index_record );
> + *pi_file_num = index_record[5];
> + }
I don't know the VDR format, but are you sure that you can always try to read 8 bytes
in non TS mode? (otherwise some valid files may be rejected).
> + return true;
> +}
> +
Regards,
--
fenrir
More information about the vlc-devel
mailing list