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

[d s]

On Sun, Oct 7, 2012 at 6:39 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Oct 02, 2012 at 07:51:37PM -0400, d s wrote:
>> On Mon, Sep 24, 2012 at 12:21 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> 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.
>> >
>> > maybe you can use avpriv_lock_avformat() and avpriv_unlock_avformat()
>> > instead of this or the whole mutex
>> >
>> I read the code, and it seems the user must create a lock manager,
>> without which those functions are NOPs. Is it safe to assume that
>> ff_lockmgr_cb will exist in every situation where multiple avformat
>> threads are running?
>
> id say that a user application that doesnt set the callback and uses
> the avxsynth demuxer from multiple threads is buggy.
>
>
> [...]
>> typedef struct {
>>     AVS_ScriptEnvironment *env;
>>     AVS_Clip *clip;
>>     const AVS_VideoInfo *vi;
>
>>
>>     // avisynth_read_packet_video() iterates over this.
>>     int n_planes;
>
> comments should ideally be doxygen compatible
>
>
> [...]
>> static void av_cold avisynth_free_library(void) {
>>     avs_ref_count--;
>>     if (avs_ref_count > 0)
>>         return;
>>     FreeLibrary(avs_library->library);
>
>>     av_free(avs_library);
>>     avs_library = NULL;
>
> you can use av_freep() to free and zero a pointer
>
>
> [...]
>> // Create AVStream from audio and video data.
>> static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st) {
>>     AvisynthContext *avs = s->priv_data;
>>     int planar = 0; // 0: packed, 1: YUV, 2: Y8
>>
>>     st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
>>     st->codec->codec_id = CODEC_ID_RAWVIDEO;
>>     st->codec->width = avs->vi->width;
>>     st->codec->height = avs->vi->height;
>>
>>     st->time_base = (AVRational) {avs->vi->fps_denominator, avs->vi->fps_numerator};
>>     st->avg_frame_rate = (AVRational) {avs->vi->fps_numerator, avs->vi->fps_denominator};
>>     st->start_time = 0;
>>     st->duration = avs->vi->num_frames;
>>     st->nb_frames = avs->vi->num_frames;
>>
>>     switch (avs->vi->pixel_type) {
>>     case AVS_CS_BGR24:
>>         st->codec->pix_fmt = PIX_FMT_BGR24;
>>         break;
>>     case AVS_CS_BGR32:
>>         st->codec->pix_fmt = PIX_FMT_BGRA;
>>         break;
>>     case AVS_CS_YUY2:
>>         st->codec->pix_fmt = PIX_FMT_YUYV422;
>>         break;
>>     case AVS_CS_YV12:
>>         st->codec->pix_fmt = PIX_FMT_YUV420P;
>>         planar = 1;
>>         break;
>>     case AVS_CS_I420: // Is this even used anywhere?
>>         st->codec->pix_fmt = PIX_FMT_YUV420P;
>>         planar = 1;
>>         break;
>>     default:
>>         FAIL_IF_MSG(1, fail, "unknown Avisynth colorspace %d\n", avs->vi->pixel_type);
>>     }
>>     switch (planar) {
>
>>     case 2: // Y
>
> this is unreachable, did you forget something or is thie work in
> progress ?
>
>
> [...]
>> static int avisynth_create_stream_audio(AVFormatContext *s, AVStream *st) {
>>     AvisynthContext *avs = s->priv_data;
>>
>>     st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
>>     st->codec->sample_rate = avs->vi->audio_samples_per_second;
>>     st->codec->channels = avs->vi->nchannels;
>>     st->time_base = (AVRational) {1, avs->vi->audio_samples_per_second};
>>
>>     switch (avs->vi->sample_type) {
>>     case AVS_SAMPLE_INT8:
>>         st->codec->codec_id = CODEC_ID_PCM_U8;
>>         break;
>>     case AVS_SAMPLE_INT16:
>>         st->codec->codec_id = CODEC_ID_PCM_S16LE;
>>         break;
>>     case AVS_SAMPLE_INT24:
>>         st->codec->codec_id = CODEC_ID_PCM_S24LE;
>>         break;
>>     case AVS_SAMPLE_INT32:
>>         st->codec->codec_id = CODEC_ID_PCM_S32LE;
>>         break;
>>     case AVS_SAMPLE_FLOAT:
>>         st->codec->codec_id = CODEC_ID_PCM_F32LE;
>>         break;
>>     default:
>>         av_log(s, AV_LOG_ERROR, "unknown avisynth sample type %d\n", avs->vi->sample_type);
>>         goto fail;
>>     }
>>     return 0;
>>
>> fail:
>>     avs->error = 1;
>>     return AVISYNTH_ERROR;
>
> the goto could be avoided here by simply inlining the code at its only
> caller, not sure if you have expect this to be used from another spot
> in the future ?
>
>
> [...]
>> // Copy Avisynth clip data into an AVPacket.
>> static int avisynth_read_packet_video(AVFormatContext *s, AVPacket *pkt) {
>>     AvisynthContext *avs = s->priv_data;
>>     AVS_VideoFrame *frame;
>>     unsigned char*  dst_p;
>>     const unsigned char* src_p;
>>     int i, plane, rowsize, planeheight, pitch;
>>     const char* error;
>>
>>     if (avs->curr_frame >= avs->vi->num_frames)
>>         return AVISYNTH_EOF;
>
> error codes in libavformat should be AVERROR* compatible, that is
> AVERROR_EOF here. Otherwise the return code is hard to interpret
> when different demuxers uses different systems
>
>
>
>>
>>     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;
>
> i dont know exactly where the numbers come from but if the
> multiplications could overflow, then this has to be checked for
> due to the implications on the malloc below
>
>>     pkt->data = av_malloc(pkt->size);
>
>
>>     if (!pkt->data)
>>         return AVISYNTH_ERROR;
>>
>>     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 (i = 0; i < avs->n_planes; i++) {
>>         plane = avs->planes[i];
>>         src_p = avs_get_read_ptr_p(frame, plane);
>>         rowsize = avs_get_row_size_p(frame, plane);
>>         planeheight = avs_get_height_p(frame, plane);
>>         pitch = avs_get_pitch_p(frame, plane);
>>
>>         avs_library->avs_bit_blt(avs->env, dst_p, rowsize, src_p, pitch, rowsize, planeheight);
>>         dst_p += rowsize * planeheight;
>>     }
>>
>>     avs_library->avs_release_video_frame(frame);
>>     avs->curr_frame++;
>>     return 0;
>>
>
>> fail:
>>     av_free(pkt->data);
>
> the pointer should be zeroed here for saftey, this applies to more
> places
>
>
> [...]
>> static int avisynth_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp, int flags) {
>>     AvisynthContext *avs = s->priv_data;
>>     AVStream *st;
>>     double fps, samplerate;
>>
>>     // Don't continue on error.
>>     if (avs->error)
>>         return AVISYNTH_ERROR;
>>
>>     // Make everything a double to simplify calculations.
>>     fps = ((double) avs->vi->fps_numerator / (double) avs->vi->fps_denominator);
>>     samplerate = avs->vi->audio_samples_per_second;
>>
>>     st = s->streams[stream_index];
>>     if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>>         // Avisynth frame counts are signed int.
>>         if ((timestamp >= avs->vi->num_frames) || (timestamp > INT_MAX) || (timestamp < 0))
>>             return AVISYNTH_EOF;
>>         avs->curr_frame = timestamp;
>
>>         avs->curr_sample = timestamp * samplerate / fps;
>
> you can calculate this exactly with av_rescale_q() avoiding floats and
> possibly different rounding on different platforms
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I am the wisest man alive, for I know one thing, and that is that I know
> nothing. -- Socrates

I will make the changes you suggest.

One thing I noticed when reviewing my code though was that
avisynth_read_packet reads video faster than it does audio. This
probably means the audio is not being kept in sync and needs to be
fixed. On this subject though, currently the code returns EOF when
either the audio or video end is reached (probably video). How should
I handle uneven stream lengths? Is it OK to return an empty packet?