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

[d s]

Oops, I accidentally replied to michaeln instead of the list.

On Sun, Sep 23, 2012 at 4:46 PM, d s <avxsynth.testing at gmail.com> wrote:
> On Sun, Sep 23, 2012 at 2:57 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> [...]
>>> // Platform-specific directives for Avisynth vs Avxsynth.
>>> #ifdef _WIN32
>>
>>>   #include "../libavcodec/w32pthreads.h"
>>
>> that ../ looks wrong
>>
> What is the correct way of referencing libavcodec from libavformat?
>
>>
>> [...]
>>> #define FAIL_IF_MSG(cond, label, ...) \
>>>         if ( cond ) {av_log(s, AV_LOG_ERROR, __VA_ARGS__); goto label;}
>>
>> tabs are forbidden in ffmpeg git
>>
>>
> I will convert the tabs to spaces.
>
>
>>>         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
>>
>>
> OK.
>
>> [...]
>>> 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
>>
>>
> This bothers me too, but I'm not sure how to approach this. I don't
> want to construct the mutex unless the demuxer is actually being used,
> but if I don't have an atomic operation somewhere, there could be a
> race condition calling pthread_mutex_init.
>
>
Actually, now that I think of it, this code doesn't do what I think it
does, so it will need to be rewritten.


>>>                 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
> I will replace goto --> return statements. Regarding the mutex though,
> trying to unlock a mutex after a constructor failure or similar is an
> undefined behavior unless I add a bunch more code to handle the return
> value. I would rather just return a hard error if such a thing is
> possible in avformat.
>
>
>>
>>
>>
>>
>>
>>>         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
> I may be misunderstanding something, but how could curr_stream
> overflow? s->nb_streams can only be 1 or 2 in Avisynth. curr_stream
> goes 0, 1, 0, 1, 0, 1, ...