[FFmpeg-devel] [PATCH] avformat: avisynth demuxer rewrite

[Michael Niedermayer]

Hi

On Sat, Sep 22, 2012 at 04:27:11PM -0400, d s wrote:
> Dear ffmpeg-devel,
> 
> The current Avisynth demuxer is misnamed; rather, it would be more
> appropriately described as a VfW demuxer. This email contains a
> rewrite of the demuxer that uses the Avisynth library directly, with
> the following benefits:
> 
> 1) useful error messages instead of video clips with error text
> 2) no dependency on VfW, and hence no requirement for the related SDKs/libraries
> 3) able to support AvxSynth (https://github.com/avxsynth/avxsynth/wiki) on Linux
> 
> I have tested the demuxer on both Windows and Linux and verified that
> I was able to ffprobe YV12, YUY2, and RGB32 scripts as well as encode
> them to rawvideo.
> 
> Review and comments would be appreciated.

a quick review from me below, there probably will be comments from
others too ...


[...]
> // Platform-specific directives for Avisynth vs Avxsynth.
> #ifdef _WIN32

>   #include "../libavcodec/w32pthreads.h"

that ../ looks wrong


[...]
> #define FAIL_IF_MSG(cond, label, ...) \
>         if ( cond ) {av_log(s, AV_LOG_ERROR, __VA_ARGS__); goto label;}

tabs are forbidden in ffmpeg git


[...]
> // Lock mutex before calling avisynth_load_library() and avisynth_free_library().
> static int avisynth_load_library(void) {
>         if (avs_ref_count++ != 0)
>                 return 0;
> 
>         avs_library = av_mallocz(sizeof(AvisynthLibrary));
>         if (!avs_library)
>                 goto malloc_fail;
>         avs_library->library = LoadLibrary(AVISYNTH_LIB);
>         if (!avs_library->library)
>                 goto init_fail;
> 
> #define LOAD_AVS_FUNC(name, continue_on_fail) \
> { \
>         avs_library->name = (void*)GetProcAddress(avs_library->library, #name); \
>         if(!continue_on_fail && !avs_library->name) \
>                 goto fail; \
> }
>          LOAD_AVS_FUNC(avs_create_script_environment, 0);
>         LOAD_AVS_FUNC(avs_delete_script_environment, 0);
>         LOAD_AVS_FUNC(avs_get_error, 1); // New to Avisynth 2.6
>         LOAD_AVS_FUNC(avs_clip_get_error, 0);
>         LOAD_AVS_FUNC(avs_invoke, 0);
>         LOAD_AVS_FUNC(avs_release_value, 0);
>         LOAD_AVS_FUNC(avs_get_video_info, 0);
>         LOAD_AVS_FUNC(avs_take_clip, 0);
>         LOAD_AVS_FUNC(avs_release_clip, 0);
>         LOAD_AVS_FUNC(avs_bit_blt, 0);
>         LOAD_AVS_FUNC(avs_get_audio, 0);
>         LOAD_AVS_FUNC(avs_get_frame, 0);
>         LOAD_AVS_FUNC(avs_release_video_frame, 0);
> #undef LOAD_AVS_FUNC
>         return 0;
> 
> fail:
>         FreeLibrary(avs_library->library);
> init_fail:
>         av_free(avs_library);
> malloc_fail:
>         avs_ref_count--;
>         return AVISYNTH_ERROR;
> }
> 
> static void avisynth_free_library(void) {
>         avs_ref_count--;
>         if (avs_ref_count > 0)
>                 return;
>         FreeLibrary(avs_library->library);
>         av_free(avs_library);
>         avs_library = NULL;
> }

rarely used functions should be marked with av_cold


[...]
> // Copy Avisynth clip data into an AVPacket.
> static int avisynth_read_packet_video(AVFormatContext *s, AVPacket *pkt) {

the comment could be made doxygen compatible

    
>         AvisynthContext *avs = s->priv_data;
>         AVS_VideoFrame *frame;
>         unsigned char*  dst_p;
>         const unsigned char* src_p;
>         int plane, rowsize, planeheight, pitch;
>         const char* error;
> 
>         if (avs->curr_frame >= avs->vi->num_frames)
>                 return AVISYNTH_EOF;
> 
>         pkt->pts = avs->curr_frame;
>         pkt->dts = avs->curr_frame;
>         pkt->duration = 1;
> 
>         pkt->size = avs->vi->width * avs->vi->height * avs_bits_per_pixel(avs->vi) / 8;
>         pkt->data = av_malloc(pkt->size);
>         if (!pkt->data)
>                 goto malloc_fail;
> 
>         frame = avs_library->avs_get_frame(avs->clip, avs->curr_frame);
>         error = avs_library->avs_clip_get_error(avs->clip);
>         FAIL_IF_MSG(error, fail, error);
> 
>         dst_p = pkt->data;

>         for (int i = 0; i < avs->n_planes; i++) {

please move the "int i" up, some compilers dont support this syntax


[...]
> static int avisynth_read_header(AVFormatContext *s) {
>         // Make sure avs_mutex isn't initialized multiple times.

>         if (__sync_bool_compare_and_swap(&avs_mutex_ready, 0, 1)) {

i dont think this is portable

    
>                 avs_mutex = av_malloc(sizeof(pthread_mutex_t));
>                 if (!avs_mutex)
>                         goto init_fail;
>                 if (pthread_mutex_init(avs_mutex, NULL))
>                         goto init_fail;
>         }
> 
>         if (pthread_mutex_lock(avs_mutex))
>                 goto init_fail;
>         if (avisynth_load_library())
>                 goto init_fail;

please dont goto to a single return statement, this obfuscates the
code. And it also forgets unlocking the mutex in this case





>         if (avisynth_open_file(s))
>                 goto fail;
>         if (avisynth_create_stream(s))
>                 goto fail;
>         if (pthread_mutex_unlock(avs_mutex))
>                 goto fail;
>         return 0;
> 
> fail:
>         avisynth_free_library();
>         pthread_mutex_unlock(avs_mutex);
> init_fail:
>         return AVISYNTH_ERROR;
> }
> 
> static int avisynth_read_packet(AVFormatContext *s, AVPacket *pkt) {
>         AvisynthContext *avs = s->priv_data;
>         AVStream *st;
> 
>         // Don't continue on error.
>         if (avs->error)
>                 return AVISYNTH_ERROR;
> 

>         pkt->stream_index = avs->curr_stream++;
>         avs->curr_stream %= s->nb_streams;

this will fail when curr_stream overflows


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120923/c934ff82/attachment.asc>