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

[Michael Niedermayer]

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
-------------- 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/20121008/61784613/attachment.asc>