On Wed, Sep 4, 2013 at 1:34 PM, Diego Biurrun <[email protected]> wrote:
> On Wed, Sep 04, 2013 at 11:29:02AM -0400, Stephen Hutchinson wrote:
> > --- a/libavformat/avisynth.c
> > +++ b/libavformat/avisynth.c
> > @@ -21,203 +21,660 @@
> >
> > #include "avformat.h"
> > #include "internal.h"
> > -#include "riff.h"
> > -
> > -#include <windows.h>
> > -#include <vfw.h>
> > +#include "libavcodec/internal.h"
> > +
> > +// Enable function pointer definitions for runtime loading.
> > +#define AVSC_NO_DECLSPEC
> > +
> > +// Shut up error messages.
> > +// avisynth_c.h contains inline functions that call these functions.
> > +#undef malloc
> > +#undef free
> > +#undef printf
>
> These comments appear outdated.
>
It does appear to be that way. I did notice that removing the set of
undefs didn't throw any warnings or errors anymore. It's been removed.
>
> > +// Platform-specific directives for AviSynth vs AvxSynth.
> > +#ifdef _WIN32
> > + #include <windows.h>
> > + #undef EXTERN_C
> > + #include <avisynth/avisynth_c.h>
> > + #include <avisynth/avisynth_c_25.h>
> > + #define AVISYNTH_LIB "avisynth"
> > +#else
> > + #include <dlfcn.h>
> > + #include <avxsynth/avxsynth_c.h>
> > + #if defined (__APPLE__)
> > + #define AVISYNTH_LIB "libavxsynth.dylib"
> > + #else
> > + #define AVISYNTH_LIB "libavxsynth.so"
> > + #endif
> > +
> > + #define LoadLibrary(x) dlopen(x, RTLD_NOW | RTLD_GLOBAL)
> > + #define GetProcAddress dlsym
> > + #define FreeLibrary dlclose
> > +#endif
>
> What is this ifdeffery for?
>
AviSynth and AvxSynth use different headers, and since AviSynth is
Windows-only and AvxSynth is Linux and OSX only, those need to be
differentiated. Further, the lib name for AvxSynth is different between
Linux and OSX (which is the only difference there, since they both use
dlopen).
>
> > typedef struct {
> > + void *library;
> > +#define AVSC_DECLARE_FUNC(name) name##_func name
> > + AVSC_DECLARE_FUNC(avs_bit_blt);
> > + AVSC_DECLARE_FUNC(avs_clip_get_error);
> > + AVSC_DECLARE_FUNC(avs_create_script_environment);
> > + AVSC_DECLARE_FUNC(avs_delete_script_environment);
> > + AVSC_DECLARE_FUNC(avs_get_audio);
> > + AVSC_DECLARE_FUNC(avs_get_error);
> > + AVSC_DECLARE_FUNC(avs_get_frame);
> > + AVSC_DECLARE_FUNC(avs_get_version);
> > + AVSC_DECLARE_FUNC(avs_get_video_info);
> > + AVSC_DECLARE_FUNC(avs_invoke);
> > + AVSC_DECLARE_FUNC(avs_release_clip);
> > + AVSC_DECLARE_FUNC(avs_release_value);
> > + AVSC_DECLARE_FUNC(avs_release_video_frame);
> > + AVSC_DECLARE_FUNC(avs_take_clip);
> > +#undef AVSC_DECLARE_FUNC
> > +} AviSynthLibrary;
>
> Don't anonymously declare structs, this can cause problems.
>
Done.
>
> > +static const int avs_planes_packed[1] = {0};
> > +static const int avs_planes_grey[1] = {AVS_PLANAR_Y};
> > +static const int avs_planes_yuv[3] = {AVS_PLANAR_Y, AVS_PLANAR_U,
> AVS_PLANAR_V};
>
> spaces inside {}
>
Done.
>
> > +static av_cold int avisynth_load_library(void)
> > {
> > + avs_library = av_mallocz(sizeof(AviSynthLibrary));
> > + if (!avs_library)
> > + return AVERROR_UNKNOWN;
>
> AVERROR(ENOMEM)
>
Done. Is this the only place that ENOMEM needs to be used, or should it be
used anywhere an av_malloc is? I assumed the later, so there's a couple of
other spots I changed too.
>
> > + 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) \
>
> if (
>
Done.
>
> > +// Note that avisynth_context_create and avisynth_context_destroy
> > +// do not allocate or free the actual context! That is taken care of
> > +// by libavformat.
> > +static av_cold int avisynth_context_create(AVFormatContext *s)
> > +{
> > + AviSynthContext *avs = (AviSynthContext *)s->priv_data;
>
> Looks like a pointless void* cast.
>
Done. Also removed the cast in avisynth_open_file that appeared to do the
same thing.
>
> > +// 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 = AV_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;
>
> Align some of these =.
>
> more below
>
Done.
>
> > +static int avisynth_open_file(AVFormatContext *s)
> > {
> > + AviSynthContext *avs = (AviSynthContext *)s->priv_data;
> > + AVS_Value arg, val;
> > + int ret;
> > +#ifdef _WIN32
> > + char filename_ansi[MAX_PATH * 4];
> > + wchar_t filename_wc[MAX_PATH * 4];
> > +#endif
> > +
> > + if (ret = avisynth_context_create(s))
> > + return ret;
> > +
> > +#ifdef _WIN32
> > + // Convert UTF-8 to ANSI code page
> > + MultiByteToWideChar(CP_UTF8, 0, s->filename, -1, filename_wc,
> MAX_PATH * 4);
> > + WideCharToMultiByte(CP_THREAD_ACP, 0, filename_wc, -1,
> filename_ansi, MAX_PATH * 4, NULL, NULL);
> > + arg = avs_new_value_string(filename_ansi);
> > +#else
> > + arg = avs_new_value_string(s->filename);
> > +#endif
>
> What is this about?
>
That is basically the following change that was committed in 2012 against
the VFW-based demuxer, but restructured against the new one:
http://git.libav.org/?p=libav.git;a=commit;h=5c742005fb7854dcfaa9f0efb65fd36a63ceaa2b
It's ifdeffed because it's a Windows-specific problem.
>
> > +static void avisynth_next_stream(AVFormatContext *s, AVStream **st,
> > + AVPacket *pkt, int *discard)
>
> Indentation is off, more below.
>
Done.
>
> > + // Define the bpp values for the new AviSynth 2.6 colorspaces
> > + if (avs_is_yv24(avs->vi)) {
> > + bits = 24;
> > + } else if (avs_is_yv16(avs->vi)) {
> > + bits = 16;
> > + } else if (avs_is_yv411(avs->vi)) {
> > + bits = 12;
> > + } else if (avs_is_y8(avs->vi)) {
> > + bits = 8;
> > + } else {
> > + bits = avs_bits_per_pixel(avs->vi);
> > + }
>
> Drop the {}
>
Done.
>
> > +#ifdef _WIN32
> > + if (avs_library->avs_get_version(avs->clip) == 3) {
> > + rowsize = avs_get_row_size_p_25(frame, plane);
> > + planeheight = avs_get_height_p_25(frame, plane);
> > + } else {
> > + rowsize = avs_get_row_size_p(frame, plane);
> > + planeheight = avs_get_height_p(frame, plane);
> > + }
> > +#else
> > + rowsize = avs_get_row_size_p(frame, plane);
> > + planeheight = avs_get_height_p(frame, plane);
> > +#endif
>
> The else and the #else case look identical.
>
They are identical, because of the situation with
AVISYNTH_INTERFACE_VERSION. Even though AvxSynth uses its own header (and
those functions are identical to 2.6's, not 2.5.8's - this is one of those
things that apparently got backported from 2.6), its INTERFACE VERSION is
3, just like 2.5.8. So if it wasn't ifdeffed, the version check would lump
AvxSynth in with 2.5.8 erroneously, which would then cause AvxSynth to
crash the same way that 2.5.8 would if the compatibility header wasn't
brought in to provide the _25 suffixed functions there.
>
> > + // If either stream reaches EOF, try to read the other one before
> giving up.
> > + avisynth_next_stream(s, &st, pkt, &discard);
> > + if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> > + ret = avisynth_read_packet_video(s, pkt, discard);
> > + if (ret == AVERROR_EOF && avs_has_audio(avs->vi)) {
> > + avisynth_next_stream(s, &st, pkt, &discard);
> > + return avisynth_read_packet_audio(s, pkt, discard);
> > + }
> > + return ret;
> > + } else {
> > + ret = avisynth_read_packet_audio(s, pkt, discard);
> > + if (ret == AVERROR_EOF && avs_has_video(avs->vi)) {
> > + avisynth_next_stream(s, &st, pkt, &discard);
> > + return avisynth_read_packet_video(s, pkt, discard);
> > + }
> > + return ret;
> > + }
> > }
>
> Factor out the return.
>
Done, I think.
>> + if (!avs_is_clip(val)) {
>> + av_log(s, AV_LOG_ERROR, "%s\n", "AviSynth script did not return
a clip");
>
>Just use the string directly.
Would the string in question be avs_is_clip or avs_is_error?
I'll have to rework some of this against the revised version of patch
posted later.
There is one little issue in that if the script's audio ends before the
video stream does, the logging stops reporting the current frame/time until
encoding is finished, but the outputted stream seems to be fine. This even
occurs with the original version of patch against libav, so it doesn't seem
to be something introduced with the revisions. It doesn't affect ffmpeg,
so I think it's probably not even related to the AviSynth demuxer.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
