[vlc-devel] [PATCH] realvideo bug fix and document
Laurent Aimar
fenrir at via.ecp.fr
Thu Feb 5 20:31:38 CET 2009
Hi,
It seems to me that your patch should be splitted in two:
1. Correction to make the plugin work after the demuxer changes
2. The dll/so path changes.
I would prefer it as it is easier to review/comments and then eases
regression searches.
(Some of the following comments apply to the code that you may not have
been responsible for.)
> diff --git a/modules/codec/realvideo.c b/modules/codec/realvideo.c
> index 6e5c512..92e9da1 100644
> --- a/modules/codec/realvideo.c
> +++ b/modules/codec/realvideo.c
> @@ -31,6 +31,8 @@
> #include <vlc_vout.h>
> #include <vlc_codec.h>
>
> +#include <vlc_network.h>
> +
> #ifdef LOADER
> /* Need the w32dll loader from mplayer */
> # include <wine/winerror.h>
> @@ -89,9 +91,9 @@ struct rv_init_t
> short h;
> short unk3;
> int unk2;
> - unsigned int * subformat;
> + int subformat;
> int unk5;
> - unsigned int * format;
> + int format;
> } rv_init_t;
>
> struct decoder_sys_t
> @@ -241,19 +243,85 @@ static int InitVideo(decoder_t *p_dec)
> init_data.h = p_dec->fmt_in.video.i_height ;
> init_data.unk3 = 0;
> init_data.unk2 = 0;
> - init_data.subformat = (unsigned int*)p_vide[0];
> + init_data.subformat = ntohl( p_vide[0] );
> init_data.unk5 = 1;
> - init_data.format = (unsigned int*)p_vide[1];
> + init_data.format = ntohl( p_vide[1]);
Using GetDWBE would be better and would avoid vlc_network.h inclusion.
You have to use it directky on p_dec->fmt_in.p_extra and compute yourself the
offset (it would also remove the implicit ugly unsigned int * cast).
> /* first try to load linux dlls, if failed and we're supporting win32 dlls,
> then try to load the windows ones */
> - bool b_so_opened = false;
> +
> + p_sys->rv_handle = NULL;
>
> #ifdef WIN32
> - g_decode_path="plugins\\drv43260.dll";
>
> - if( (p_sys->rv_handle = load_syms(p_dec, g_decode_path)) )
> - b_so_opened = true;
> + const char psz_paths[MAX_PATH*3] ;
The const should be removed as you modify it.
Now using an array would be really better, like
char ppsz_path[3][MAX_PATH+1]
as it would simplify a lot the code.
> + {
> + HKEY h_key;
> + DWORD i_type, i_data = MAX_PATH + 1;
> + char psz_codecs[MAX_PATH+1];
> + char * p_data ;
> + char * p_end = psz_paths;
With char ppsz_path[3][MAX_PATH+1] they can be removed I think.
> +
> + p_data = psz_codecs;
> + if ( RegOpenKeyEx( HKEY_CLASSES_ROOT,
> + _T("Software\\RealNetworks\\Preferences\\DT_Codecs"),
> + 0, KEY_READ, &h_key ) == ERROR_SUCCESS )
> + {
> + if ( RegQueryValueEx( h_key, _T(""), 0, &i_type,
> + (LPBYTE) p_data, &i_data ) == ERROR_SUCCESS
> + && i_type == REG_SZ )
> + {
> + int i_len = strlen( p_data );
> + if ( i_len && p_data[i_len-1] == '\\' )
> + p_data[i_len - 1] = '\0';
> + memcpy( p_end, p_data, strlen( p_data) + 1 );
> + p_end += strlen( p_data ) + 1;
> + }
> + RegCloseKey( h_key );
> + }
> +
> + p_data = psz_codecs;
> + if ( RegOpenKeyEx( HKEY_CLASSES_ROOT,
> + _T("Helix\\HelixSDK\\10.0\\Preferences\\DT_Codecs"),
> + 0, KEY_READ, &h_key ) == ERROR_SUCCESS )
> + {
> + if ( RegQueryValueEx( h_key, _T(""), 0, &i_type,
> + (LPBYTE) p_data, &i_data ) == ERROR_SUCCESS
> + && i_type == REG_SZ )
> + {
> + int i_len = strlen( p_data );
> + if ( i_len && p_data[i_len-1] == '\\' )
> + p_data[i_len - 1] = '\0';
> + memcpy( p_end, p_data, strlen( p_data) + 1 );
> + p_end += strlen( p_data ) + 1;
> + }
> + RegCloseKey( h_key );
> + }
> +
> + memcpy( p_end, ".\0codecs\0plugins\0\0\0", 20 );
> + }
> +
> + for( size_t i = 0; psz_paths[i]; i += strlen( psz_paths + i ) + 1 )
> + {
> + if( asprintf( &g_decode_path, "%s/drv4.dll", psz_paths + i ) != -1 )
> + {
> + p_sys->rv_handle = load_syms(p_dec, g_decode_path);
> + free( g_decode_path );
> + }
> + if( p_sys->rv_handle )
> + break;
> +
> + if( asprintf( &g_decode_path, "%s/drv43260.dll", psz_paths + i ) != -1 )
> + {
> + p_sys->rv_handle = load_syms(p_dec, g_decode_path);
> + free( g_decode_path );
> + }
> + if( p_sys->rv_handle )
> + break;
> +
> + /*msg_Dbg( p_dec, "Cannot load real decoder library: %s", g_decode_path);*/
> + }
> #else
> static const char psz_paths[] =
> {
> @@ -272,7 +340,10 @@ static int InitVideo(decoder_t *p_dec)
> "/usr/lib64/RealPlayer10/codecs\0"
> "/usr/lib64/RealPlayer10GOLD/codecs\0"
> "/usr/local/lib/codecs\0"
> - "\0"
> + ".\0"
> + "codecs\0"
> + "plugins\0"
> + "\0\0"
> };
Same here (array).
>
> for( size_t i = 0; psz_paths[i]; i += strlen( psz_paths + i ) + 1 )
> @@ -283,10 +354,7 @@ static int InitVideo(decoder_t *p_dec)
> free( g_decode_path );
> }
> if( p_sys->rv_handle )
> - {
> - b_so_opened = true;
> break;
> - }
>
> if( asprintf( &g_decode_path, "%s/drv3.so.6.0", psz_paths + i ) != -1 )
> {
> @@ -294,16 +362,13 @@ static int InitVideo(decoder_t *p_dec)
> free( g_decode_path );
> }
> if( p_sys->rv_handle )
> - {
> - b_so_opened = true;
> break;
> - }
>
> - msg_Dbg( p_dec, "Cannot load real decoder library: %s", g_decode_path);
> + /*msg_Dbg( p_dec, "Cannot load real decoder library: %s", g_decode_path);*/
> }
> #endif
>
> - if(!b_so_opened )
> + if( p_sys->rv_handle == NULL )
> {
> msg_Err( p_dec, "Cannot any real decoder library" );
> free( p_sys );
> @@ -321,24 +386,27 @@ static int InitVideo(decoder_t *p_dec)
> result=(*rvyuv_init)(&init_data, &p_sys->handle);
> if (result)
> {
> - msg_Err( p_dec, "Cannot Init real decoder library: %s", g_decode_path);
> + msg_Err( p_dec, "Cannot Init real decoder, error code: %X", result );
> free( p_sys );
> return VLC_EGENERIC;
> }
>
> + msg_Dbg( p_dec, "codec id: 0x%8X subid: 0x%8X\n", ntohl(p_vide[1]), ntohl(p_vide[0]) );
> +
> /* setup rv30 codec (codec sub-type and image dimensions): */
> /*if ( p_dec->fmt_in.i_codec == VLC_FOURCC('R','V','3','0') )*/
> - if (p_vide[1]>=0x20200002)
> + if ( ntohl( p_vide[1])>=0x20200002)
Why not storing the ntohl( p_vide[1]) (same with p_vide[0]) values in a local variables ?
> {
> - int i, cmsg_cnt;
> + int cmsg_cnt;
> + unsigned char * p_extrahdr = (unsigned char *) p_vide ;
uint8_t
> uint32_t cmsg24[16]={p_dec->fmt_in.video.i_width,p_dec->fmt_in.video.i_height};
> - cmsg_data_t cmsg_data={0x24,1+(p_vide[1]&7), &cmsg24[0]};
> - cmsg_cnt = (p_vide[1]&7)*2;
> + cmsg_data_t cmsg_data={0x24,1+(p_extrahdr[1]&7), &cmsg24[0]};
> + cmsg_cnt = (p_extrahdr[1]&7)*2;
> if (i_vide - 8 < cmsg_cnt) {
> cmsg_cnt = i_vide - 8;
> }
> - for (i = 0; i < cmsg_cnt; i++)
> - cmsg24[2+i] = p_vide[8+i]*4;
> + for (int i = 0; i < cmsg_cnt; i++)
> + cmsg24[2+i] = p_extrahdr[8+i]*4;
> #ifdef WIN32
> if (dll_type == 1)
> (*wrvyuv_custom_message)(&cmsg_data,p_sys->handle);
> @@ -349,7 +417,7 @@ static int InitVideo(decoder_t *p_dec)
> /*
> es_format_Init( &p_dec->fmt_out, VIDEO_ES, VLC_FOURCC( 'Y','V','1','2' ));
> es_format_Init( &p_dec->fmt_out, VIDEO_ES, VLC_FOURCC( 'Y','U','Y','2' ));
> - */
> + */
> es_format_Init( &p_dec->fmt_out, VIDEO_ES, VLC_FOURCC( 'I', '4', '2', '0'));
>
> p_dec->fmt_out.video.i_width = p_dec->fmt_in.video.i_width;
> @@ -467,25 +535,20 @@ static picture_t *DecodeVideo( decoder_t *p_dec, block_t **pp_block )
> if ( p_pic )
> {
> unsigned int transform_out[5];
> - dp_hdr_t dp_hdr;
> transform_in_t transform_in;
> - uint32_t pkg_len = ((uint32_t*)p_block->p_buffer)[0];
> - unsigned char* dp_data=((unsigned char*)p_block->p_buffer)+8;
> - uint32_t* extra=(uint32_t*)(((char*)p_block->p_buffer)+8+pkg_len);
> - uint32_t img_size;
> -
>
> - dp_hdr.len = pkg_len;
> - dp_hdr.chunktab = 8 + pkg_len;
> - dp_hdr.chunks = ((uint32_t*)p_block->p_buffer)[1]-1;
> - dp_hdr.timestamp = i_pts;
> + uint8_t i_chunks = p_block->p_buffer[0];
> + uint32_t pkg_len = p_block->i_buffer - 2*4*(i_chunks + 1) - 1;
i_ prefix.
> + unsigned char* dp_data=p_block->p_buffer + 2*4*(i_chunks+1) + 1;
uint8_t
> + uint32_t* extra=(uint32_t*)(p_block->p_buffer + 1);
pi_ prefix.
> + uint32_t img_size;
>
> memset(&transform_in, 0, sizeof(transform_in_t));
>
> - transform_in.len = dp_hdr.len;
> + transform_in.len = pkg_len;
> transform_in.extra = extra;
You can simply avoid the need of a local extra variable.
> - transform_in.chunks = dp_hdr.chunks;
> - transform_in.timestamp = dp_hdr.timestamp;
> + transform_in.chunks = i_chunks;
> + transform_in.timestamp = i_pts;
>
> memset (p_sys->plane, 0, p_dec->fmt_in.video.i_width * p_dec->fmt_in.video.i_height *3/2 );
>
> @@ -496,17 +559,7 @@ static picture_t *DecodeVideo( decoder_t *p_dec, block_t **pp_block )
> #endif
> result=(*rvyuv_transform)(dp_data, p_sys->plane, &transform_in, transform_out, p_sys->handle);
>
> - /* msg_Warn(p_dec, "Real Size %d X %d", transform_out[3],
> - transform_out[4]); */
> - /* some bug rm file will print the messages :
> - [00000551] realvideo decoder warning: Real Size 320 X 240
> - [00000551] realvideo decoder warning: Real Size 480 X 272
> - [00000551] realvideo decoder warning: Real Size 320 X 240
> - [00000551] realvideo decoder warning: Real Size 320 X 240
> - ...
> - so it needs fixing!
> - */
> - if ( p_sys->inited == 0 )
> + if ( result == 0 && p_sys->inited == 0 )
> {
> /* fix and get the correct image size! */
> if ( p_dec->fmt_in.video.i_width != transform_out[3]
> @@ -540,16 +593,19 @@ static picture_t *DecodeVideo( decoder_t *p_dec, block_t **pp_block )
> p_sys->inited = 1;
> }
>
> - img_size = p_dec->fmt_in.video.i_width * p_dec->fmt_in.video.i_height;
> - memcpy( p_pic->p[0].p_pixels, p_sys->plane, img_size);
> - memcpy( p_pic->p[1].p_pixels, p_sys->plane + img_size, img_size/4);
> - memcpy( p_pic->p[2].p_pixels, p_sys->plane + img_size * 5/4, img_size/4);
> - p_pic->date = i_pts ;
> + if ( result == 0 )
> + {
> + img_size = p_dec->fmt_in.video.i_width * p_dec->fmt_in.video.i_height;
> + memcpy( p_pic->p[0].p_pixels, p_sys->plane, img_size);
> + memcpy( p_pic->p[1].p_pixels, p_sys->plane + img_size, img_size/4);
> + memcpy( p_pic->p[2].p_pixels, p_sys->plane + img_size * 5/4, img_size/4);
> + p_pic->date = i_pts ;
Using vlc_memcpy here would probably be faster.
>
> /* real video frame is small( frame and frame's time-shift is short),
> so it will become late picture easier (when render-time changed)and
> droped by video-output.*/
> - p_pic->b_force = 1;
> + p_pic->b_force = 1;
Using true is prefered.
> + }
> }
>
> vlc_mutex_unlock( &rm_mutex );
Regards,
--
fenrir
More information about the vlc-devel
mailing list