| Message ID | 20180428170529.18504-1-nfxjfg@googlemail.com |
|---|---|
| State | New |
| Headers | show |
On 4/28/2018 2:05 PM, wm4 wrote: > This can "demux" .vpy files. > > Some minor code copied from other LGPL parts of FFmpeg. > > I did not found a good way to test a few of the more obscure features, > like VFR nodes, compat pixel formats, or nodes with dynamic size/format > changes. These can be easily implemented on demand. > --- > configure | 5 + > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 428 insertions(+) > create mode 100644 libavformat/vapoursynth.c [...] > +static int read_packet_vs(AVFormatContext *s, AVPacket *pkt) > +{ > + VSContext *vs = s->priv_data; > + AVStream *st = s->streams[0]; > + AVFrame *frame = NULL; > + char vserr[80]; > + const VSFrameRef *vsframe = NULL; > + const VSVideoInfo *info = vs->vsapi->getVideoInfo(vs->outnode); > + int err = 0; > + const uint8_t *src_data[4]; > + int src_linesizes[4]; > + const VSMap *props; > + int i; > + > + if (vs->current_frame >= info->numFrames) > + return AVERROR_EOF; > + > + vsframe = vs->vsapi->getFrame(vs->current_frame, vs->outnode, vserr, sizeof(vserr)); > + if (!vsframe) { > + av_log(s, AV_LOG_ERROR, "Error getting frame: %s\n", vserr); > + err = AVERROR_EXTERNAL; > + goto end; > + } > + > + props = vs->vsapi->getFramePropsRO(vsframe); > + > + frame = av_frame_alloc(); > + if (!frame) { > + err = AVERROR(ENOMEM); > + goto end; > + } > + > + frame->format = st->codecpar->format; > + frame->width = st->codecpar->width; > + frame->height = st->codecpar->height; > + frame->colorspace = st->codecpar->color_space; > + > + // Values according to ISO/IEC 14496-10. > + frame->colorspace = get_vs_prop_int(s, props, "_Matrix", frame->colorspace); > + frame->color_primaries = get_vs_prop_int(s, props, "_Primaries", frame->color_primaries); > + frame->color_trc = get_vs_prop_int(s, props, "_Transfer", frame->color_trc); > + > + if (get_vs_prop_int(s, props, "_ColorRange", 1) == 0) > + frame->color_range = AVCOL_RANGE_JPEG; > + > + frame->sample_aspect_ratio.num = get_vs_prop_int(s, props, "_SARNum", 0); > + frame->sample_aspect_ratio.den = get_vs_prop_int(s, props, "_SARDen", 1); > + > + av_assert0(vs->vsapi->getFrameWidth(vsframe, 0) == frame->width); > + av_assert0(vs->vsapi->getFrameHeight(vsframe, 0) == frame->height); > + > + err = av_frame_get_buffer(frame, 0); > + if (err < 0) > + goto end; > + > + for (i = 0; i < info->format->numPlanes; i++) { > + int p = vs->c_order[i]; > + src_data[i] = vs->vsapi->getReadPtr(vsframe, p); > + src_linesizes[i] = vs->vsapi->getStride(vsframe, p); > + } > + > + av_image_copy(frame->data, frame->linesize, src_data, src_linesizes, > + frame->format, frame->width, frame->height); > + > + pkt->buf = av_buffer_create((uint8_t*)frame, sizeof(*frame), > + free_frame, NULL, 0); > + if (!pkt->buf) { > + err = AVERROR(ENOMEM); > + goto end; > + } > + > + frame = NULL; // pkt owns it now > + > + pkt->data = pkt->buf->data; > + pkt->size = pkt->buf->size; > + pkt->flags |= AV_PKT_FLAG_TRUSTED; > + > + if (vs->is_cfr) > + pkt->pts = vs->current_frame; > + > + vs->current_frame++; > + > +end: > + if (err < 0) > + av_packet_unref(pkt); Unneeded. Nothing after pkt->buf is initialized can fail right now. And if av_buffer_create() fails, pkt will be untouched. > + av_frame_free(&frame); > + vs->vsapi->freeFrame(vsframe); > + return err; Shouldn't you set err to pkt->size right above the end label? > +} > + > +static int read_seek_vs(AVFormatContext *s, int stream_idx, int64_t ts, int flags) > +{ > + VSContext *vs = s->priv_data; > + > + if (!vs->is_cfr) > + return AVERROR(ENOSYS); > + > + vs->current_frame = FFMIN(FFMAX(0, ts), s->streams[0]->duration); > + return 0; > +} > + > +static av_cold int probe_vs(AVProbeData *p) > +{ > + // Explicitly do not support this. VS scripts are written in Python, and > + // can run arbitrary code on the user's system. > + return 0; > +} > + > +static const AVClass class_vs = { > + .class_name = "VapourSynth demuxer", > + .item_name = av_default_item_name, > + .option = options, > + .version = LIBAVUTIL_VERSION_INT, > +}; > + > +AVInputFormat ff_vapoursynth_demuxer = { > + .name = "vapoursynth", > + .long_name = NULL_IF_CONFIG_SMALL("VapourSynth demuxer"), > + .priv_data_size = sizeof(VSContext), > + .read_probe = probe_vs, > + .read_header = read_header_vs, > + .read_packet = read_packet_vs, > + .read_close = read_close_vs, > + .read_seek = read_seek_vs, > + .priv_class = &class_vs, > +}; >
On Sat, 28 Apr 2018 15:28:13 -0300 James Almer <jamrial@gmail.com> wrote: > On 4/28/2018 2:05 PM, wm4 wrote: > > This can "demux" .vpy files. > > > > + pkt->data = pkt->buf->data; > > + pkt->size = pkt->buf->size; > > + pkt->flags |= AV_PKT_FLAG_TRUSTED; > > + > > + if (vs->is_cfr) > > + pkt->pts = vs->current_frame; > > + > > + vs->current_frame++; > > + > > +end: > > + if (err < 0) > > + av_packet_unref(pkt); > > Unneeded. Nothing after pkt->buf is initialized can fail right now. And > if av_buffer_create() fails, pkt will be untouched. Could be good for robustness reasons, but OK, removed. > > + av_frame_free(&frame); > > + vs->vsapi->freeFrame(vsframe); > > + return err; > > Shouldn't you set err to pkt->size right above the end label? I'm not sure what you mean. err is initialized to 0, and only becomes non-0 on errors, so it doesn't need to be set here.
On 4/28/2018 3:38 PM, wm4 wrote: > On Sat, 28 Apr 2018 15:28:13 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 4/28/2018 2:05 PM, wm4 wrote: >>> This can "demux" .vpy files. >>> > > >>> + pkt->data = pkt->buf->data; >>> + pkt->size = pkt->buf->size; >>> + pkt->flags |= AV_PKT_FLAG_TRUSTED; >>> + >>> + if (vs->is_cfr) >>> + pkt->pts = vs->current_frame; >>> + >>> + vs->current_frame++; >>> + >>> +end: >>> + if (err < 0) >>> + av_packet_unref(pkt); >> >> Unneeded. Nothing after pkt->buf is initialized can fail right now. And >> if av_buffer_create() fails, pkt will be untouched. > > Could be good for robustness reasons, but OK, removed. > >>> + av_frame_free(&frame); >>> + vs->vsapi->freeFrame(vsframe); >>> + return err; >> >> Shouldn't you set err to pkt->size right above the end label? > > I'm not sure what you mean. err is initialized to 0, and only becomes > non-0 on errors, so it doesn't need to be set here. I was under the impression that read_packet() functions were supposed to return the size of the packet read on success, but it seems it's not consistent (some demuxers do, others don't).
On 4/28/2018 2:05 PM, wm4 wrote: > This can "demux" .vpy files. > > Some minor code copied from other LGPL parts of FFmpeg. > > I did not found a good way to test a few of the more obscure features, > like VFR nodes, compat pixel formats, or nodes with dynamic size/format > changes. These can be easily implemented on demand. > --- > configure | 5 + > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 428 insertions(+) > create mode 100644 libavformat/vapoursynth.c https://forum.doom9.org/showthread.php?p=1838296#post1838296 Somebody wrote a similar wrapper by properly assembling rawvideo packets instead, in a similar way we already do with avisynth. Could you use that implementation instead? It would be consistent with existing similar demuxers and much better than using wrapped_avframe.
On Sat, 28 Apr 2018 19:05:29 +0200 wm4 <nfxjfg@googlemail.com> wrote: > This can "demux" .vpy files. > > Some minor code copied from other LGPL parts of FFmpeg. > > I did not found a good way to test a few of the more obscure features, > like VFR nodes, compat pixel formats, or nodes with dynamic size/format > changes. These can be easily implemented on demand. > --- > configure | 5 + > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 428 insertions(+) > create mode 100644 libavformat/vapoursynth.c > Pushed, with some minor changes, and zero-copy frame passing.
On 5/4/2018 9:28 PM, wm4 wrote: >> --- >> configure | 5 + >> libavformat/Makefile | 1 + >> libavformat/allformats.c | 1 + >> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 428 insertions(+) >> create mode 100644 libavformat/vapoursynth.c Doesn't this merit a changelog entry, and if needed, docs? Gyan
On 5/4/2018 12:58 PM, wm4 wrote: > On Sat, 28 Apr 2018 19:05:29 +0200 > wm4 <nfxjfg@googlemail.com> wrote: > >> This can "demux" .vpy files. >> >> Some minor code copied from other LGPL parts of FFmpeg. >> >> I did not found a good way to test a few of the more obscure features, >> like VFR nodes, compat pixel formats, or nodes with dynamic size/format >> changes. These can be easily implemented on demand. >> --- >> configure | 5 + >> libavformat/Makefile | 1 + >> libavformat/allformats.c | 1 + >> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 428 insertions(+) >> create mode 100644 libavformat/vapoursynth.c >> > > Pushed, with some minor changes, and zero-copy frame passing. The first step to fix something (in this case, usage sizeof(AVFrame) outside libavutil) is not adding even more cases of the issue in question. You still can replace this with rawvideo. Someone even already wrote it for you. Lets try to abide our own ABI rules...
On Fri, 4 May 2018 13:30:38 -0300 James Almer <jamrial@gmail.com> wrote: > On 5/4/2018 12:58 PM, wm4 wrote: > > On Sat, 28 Apr 2018 19:05:29 +0200 > > wm4 <nfxjfg@googlemail.com> wrote: > > > >> This can "demux" .vpy files. > >> > >> Some minor code copied from other LGPL parts of FFmpeg. > >> > >> I did not found a good way to test a few of the more obscure features, > >> like VFR nodes, compat pixel formats, or nodes with dynamic size/format > >> changes. These can be easily implemented on demand. > >> --- > >> configure | 5 + > >> libavformat/Makefile | 1 + > >> libavformat/allformats.c | 1 + > >> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 428 insertions(+) > >> create mode 100644 libavformat/vapoursynth.c > >> > > > > Pushed, with some minor changes, and zero-copy frame passing. > > The first step to fix something (in this case, usage sizeof(AVFrame) > outside libavutil) is not adding even more cases of the issue in question. > You still can replace this with rawvideo. Someone even already wrote it > for you. > > Lets try to abide our own ABI rules... That's requires a frame copy and is not what I went through all the effort for. Why didn't you say anything when the kmsgrab code did the same thing? Or when the unwrapped frame stuff was added in the first place?
On 5/4/2018 1:51 PM, wm4 wrote: > On Fri, 4 May 2018 13:30:38 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 5/4/2018 12:58 PM, wm4 wrote: >>> On Sat, 28 Apr 2018 19:05:29 +0200 >>> wm4 <nfxjfg@googlemail.com> wrote: >>> >>>> This can "demux" .vpy files. >>>> >>>> Some minor code copied from other LGPL parts of FFmpeg. >>>> >>>> I did not found a good way to test a few of the more obscure features, >>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format >>>> changes. These can be easily implemented on demand. >>>> --- >>>> configure | 5 + >>>> libavformat/Makefile | 1 + >>>> libavformat/allformats.c | 1 + >>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 428 insertions(+) >>>> create mode 100644 libavformat/vapoursynth.c >>>> >>> >>> Pushed, with some minor changes, and zero-copy frame passing. >> >> The first step to fix something (in this case, usage sizeof(AVFrame) >> outside libavutil) is not adding even more cases of the issue in question. >> You still can replace this with rawvideo. Someone even already wrote it >> for you. >> >> Lets try to abide our own ABI rules... > > That's requires a frame copy and is not what I went through all the > effort for. > > Why didn't you say anything when the kmsgrab code did the same thing? > Or when the unwrapped frame stuff was added in the first place? I did the other day on IRC when you asked me why i was against this, if you recall, because it was then when i found out this has been the case for a long while, and why I'm now saying adding even more cases is going in the opposite direction of an actual solution. In any case, i explicitly didn't block this, and no one else seems to care, so whatever.
On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: > On 5/4/2018 1:51 PM, wm4 wrote: > > On Fri, 4 May 2018 13:30:38 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 5/4/2018 12:58 PM, wm4 wrote: > >>> On Sat, 28 Apr 2018 19:05:29 +0200 > >>> wm4 <nfxjfg@googlemail.com> wrote: > >>> > >>>> This can "demux" .vpy files. > >>>> > >>>> Some minor code copied from other LGPL parts of FFmpeg. > >>>> > >>>> I did not found a good way to test a few of the more obscure features, > >>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format > >>>> changes. These can be easily implemented on demand. > >>>> --- > >>>> configure | 5 + > >>>> libavformat/Makefile | 1 + > >>>> libavformat/allformats.c | 1 + > >>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > >>>> 4 files changed, 428 insertions(+) > >>>> create mode 100644 libavformat/vapoursynth.c > >>>> > >>> > >>> Pushed, with some minor changes, and zero-copy frame passing. > >> > >> The first step to fix something (in this case, usage sizeof(AVFrame) > >> outside libavutil) is not adding even more cases of the issue in question. > >> You still can replace this with rawvideo. Someone even already wrote it > >> for you. > >> > >> Lets try to abide our own ABI rules... > > > > That's requires a frame copy and is not what I went through all the > > effort for. > > > > Why didn't you say anything when the kmsgrab code did the same thing? > > Or when the unwrapped frame stuff was added in the first place? > > I did the other day on IRC when you asked me why i was against this, if > you recall, because it was then when i found out this has been the case > for a long while, and why I'm now saying adding even more cases is going > in the opposite direction of an actual solution. > > In any case, i explicitly didn't block this, and no one else seems to > care, so whatever. I just now realized this, no, use of sizeof(AVFrame) outside libavutil is not ok, thats a ABI/API breakage. This must be removed/reverted. In all cases that are relevant. (there can be exceptions if they are irrelevant to API/ABI compatibility) For example the "if (pkt->size < sizeof(AVFrame))" in libavcodec/wrapped_avframe.c should be ok Thanks [...]
On 5/4/2018 9:19 PM, Michael Niedermayer wrote: > On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: >> On 5/4/2018 1:51 PM, wm4 wrote: >>> On Fri, 4 May 2018 13:30:38 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> On 5/4/2018 12:58 PM, wm4 wrote: >>>>> On Sat, 28 Apr 2018 19:05:29 +0200 >>>>> wm4 <nfxjfg@googlemail.com> wrote: >>>>> >>>>>> This can "demux" .vpy files. >>>>>> >>>>>> Some minor code copied from other LGPL parts of FFmpeg. >>>>>> >>>>>> I did not found a good way to test a few of the more obscure features, >>>>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format >>>>>> changes. These can be easily implemented on demand. >>>>>> --- >>>>>> configure | 5 + >>>>>> libavformat/Makefile | 1 + >>>>>> libavformat/allformats.c | 1 + >>>>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 4 files changed, 428 insertions(+) >>>>>> create mode 100644 libavformat/vapoursynth.c >>>>>> >>>>> >>>>> Pushed, with some minor changes, and zero-copy frame passing. >>>> >>>> The first step to fix something (in this case, usage sizeof(AVFrame) >>>> outside libavutil) is not adding even more cases of the issue in question. >>>> You still can replace this with rawvideo. Someone even already wrote it >>>> for you. >>>> >>>> Lets try to abide our own ABI rules... >>> >>> That's requires a frame copy and is not what I went through all the >>> effort for. >>> >>> Why didn't you say anything when the kmsgrab code did the same thing? >>> Or when the unwrapped frame stuff was added in the first place? >> >> I did the other day on IRC when you asked me why i was against this, if >> you recall, because it was then when i found out this has been the case >> for a long while, and why I'm now saying adding even more cases is going >> in the opposite direction of an actual solution. >> >> In any case, i explicitly didn't block this, and no one else seems to >> care, so whatever. > > I just now realized this, no, use of sizeof(AVFrame) outside libavutil is not ok, > thats a ABI/API breakage. > > This must be removed/reverted. In all cases that are relevant. Fixed/adapted, not reverted. For vapoursynth and kmsgrab it should be a matter of making them output rawvideo packets. > (there can be exceptions if they are irrelevant to API/ABI compatibility) > > For example the "if (pkt->size < sizeof(AVFrame))" in libavcodec/wrapped_avframe.c > should be ok How is that one ok? Is it because sizeof(AVFrame) can grow but never shrink without a soname bump so the check is safe? Still, that's only the wrapped_avframe decoder. What about the encoder? It's used by the null and yuv4mpeg muxers, plus decklink and libndi_newtek outdevs.
On Fri, May 04, 2018 at 09:51:38PM -0300, James Almer wrote: > On 5/4/2018 9:19 PM, Michael Niedermayer wrote: > > On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: > >> On 5/4/2018 1:51 PM, wm4 wrote: > >>> On Fri, 4 May 2018 13:30:38 -0300 > >>> James Almer <jamrial@gmail.com> wrote: > >>> > >>>> On 5/4/2018 12:58 PM, wm4 wrote: > >>>>> On Sat, 28 Apr 2018 19:05:29 +0200 > >>>>> wm4 <nfxjfg@googlemail.com> wrote: > >>>>> > >>>>>> This can "demux" .vpy files. > >>>>>> > >>>>>> Some minor code copied from other LGPL parts of FFmpeg. > >>>>>> > >>>>>> I did not found a good way to test a few of the more obscure features, > >>>>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format > >>>>>> changes. These can be easily implemented on demand. > >>>>>> --- > >>>>>> configure | 5 + > >>>>>> libavformat/Makefile | 1 + > >>>>>> libavformat/allformats.c | 1 + > >>>>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> 4 files changed, 428 insertions(+) > >>>>>> create mode 100644 libavformat/vapoursynth.c > >>>>>> > >>>>> > >>>>> Pushed, with some minor changes, and zero-copy frame passing. > >>>> > >>>> The first step to fix something (in this case, usage sizeof(AVFrame) > >>>> outside libavutil) is not adding even more cases of the issue in question. > >>>> You still can replace this with rawvideo. Someone even already wrote it > >>>> for you. > >>>> > >>>> Lets try to abide our own ABI rules... > >>> > >>> That's requires a frame copy and is not what I went through all the > >>> effort for. > >>> > >>> Why didn't you say anything when the kmsgrab code did the same thing? > >>> Or when the unwrapped frame stuff was added in the first place? > >> > >> I did the other day on IRC when you asked me why i was against this, if > >> you recall, because it was then when i found out this has been the case > >> for a long while, and why I'm now saying adding even more cases is going > >> in the opposite direction of an actual solution. > >> > >> In any case, i explicitly didn't block this, and no one else seems to > >> care, so whatever. > > > > I just now realized this, no, use of sizeof(AVFrame) outside libavutil is not ok, > > thats a ABI/API breakage. > > > > This must be removed/reverted. In all cases that are relevant. > > Fixed/adapted, not reverted. For vapoursynth and kmsgrab it should be a > matter of making them output rawvideo packets. I think we meant the same thing the use of sizeof(AVFrame) outside libavutil must be removed/reverted. That can be of course through adapting/fixing the code in some form. > > > (there can be exceptions if they are irrelevant to API/ABI compatibility) > > > > For example the "if (pkt->size < sizeof(AVFrame))" in libavcodec/wrapped_avframe.c > > should be ok > > How is that one ok? Is it because sizeof(AVFrame) can grow but never > shrink without a soname bump so the check is safe? yes > > Still, that's only the wrapped_avframe decoder. What about the encoder? that one doesnt look so good unless iam missing something > It's used by the null and yuv4mpeg muxers, plus decklink and > libndi_newtek outdevs. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, 4 May 2018 21:51:38 -0300 James Almer <jamrial@gmail.com> wrote: > On 5/4/2018 9:19 PM, Michael Niedermayer wrote: > > On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: > >> On 5/4/2018 1:51 PM, wm4 wrote: > >>> On Fri, 4 May 2018 13:30:38 -0300 > >>> James Almer <jamrial@gmail.com> wrote: > >>> > >>>> On 5/4/2018 12:58 PM, wm4 wrote: > >>>>> On Sat, 28 Apr 2018 19:05:29 +0200 > >>>>> wm4 <nfxjfg@googlemail.com> wrote: > >>>>> > >>>>>> This can "demux" .vpy files. > >>>>>> > >>>>>> Some minor code copied from other LGPL parts of FFmpeg. > >>>>>> > >>>>>> I did not found a good way to test a few of the more obscure features, > >>>>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format > >>>>>> changes. These can be easily implemented on demand. > >>>>>> --- > >>>>>> configure | 5 + > >>>>>> libavformat/Makefile | 1 + > >>>>>> libavformat/allformats.c | 1 + > >>>>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> 4 files changed, 428 insertions(+) > >>>>>> create mode 100644 libavformat/vapoursynth.c > >>>>>> > >>>>> > >>>>> Pushed, with some minor changes, and zero-copy frame passing. > >>>> > >>>> The first step to fix something (in this case, usage sizeof(AVFrame) > >>>> outside libavutil) is not adding even more cases of the issue in question. > >>>> You still can replace this with rawvideo. Someone even already wrote it > >>>> for you. > >>>> > >>>> Lets try to abide our own ABI rules... > >>> > >>> That's requires a frame copy and is not what I went through all the > >>> effort for. > >>> > >>> Why didn't you say anything when the kmsgrab code did the same thing? > >>> Or when the unwrapped frame stuff was added in the first place? > >> > >> I did the other day on IRC when you asked me why i was against this, if > >> you recall, because it was then when i found out this has been the case > >> for a long while, and why I'm now saying adding even more cases is going > >> in the opposite direction of an actual solution. > >> > >> In any case, i explicitly didn't block this, and no one else seems to > >> care, so whatever. > > > > I just now realized this, no, use of sizeof(AVFrame) outside libavutil is not ok, > > thats a ABI/API breakage. > > > > This must be removed/reverted. In all cases that are relevant. > > Fixed/adapted, not reverted. For vapoursynth and kmsgrab it should be a > matter of making them output rawvideo packets. Why don't you get it? We don't want to copy and then end up with unaligned shit.
On 5/5/18, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 4 May 2018 21:51:38 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 5/4/2018 9:19 PM, Michael Niedermayer wrote: >> > On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: >> >> On 5/4/2018 1:51 PM, wm4 wrote: >> >>> On Fri, 4 May 2018 13:30:38 -0300 >> >>> James Almer <jamrial@gmail.com> wrote: >> >>> >> >>>> On 5/4/2018 12:58 PM, wm4 wrote: >> >>>>> On Sat, 28 Apr 2018 19:05:29 +0200 >> >>>>> wm4 <nfxjfg@googlemail.com> wrote: >> >>>>> >> >>>>>> This can "demux" .vpy files. >> >>>>>> >> >>>>>> Some minor code copied from other LGPL parts of FFmpeg. >> >>>>>> >> >>>>>> I did not found a good way to test a few of the more obscure >> >>>>>> features, >> >>>>>> like VFR nodes, compat pixel formats, or nodes with dynamic >> >>>>>> size/format >> >>>>>> changes. These can be easily implemented on demand. >> >>>>>> --- >> >>>>>> configure | 5 + >> >>>>>> libavformat/Makefile | 1 + >> >>>>>> libavformat/allformats.c | 1 + >> >>>>>> libavformat/vapoursynth.c | 421 >> >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++ >> >>>>>> 4 files changed, 428 insertions(+) >> >>>>>> create mode 100644 libavformat/vapoursynth.c >> >>>>>> >> >>>>> >> >>>>> Pushed, with some minor changes, and zero-copy frame passing. >> >>>> >> >>>> The first step to fix something (in this case, usage sizeof(AVFrame) >> >>>> outside libavutil) is not adding even more cases of the issue in >> >>>> question. >> >>>> You still can replace this with rawvideo. Someone even already wrote >> >>>> it >> >>>> for you. >> >>>> >> >>>> Lets try to abide our own ABI rules... >> >>> >> >>> That's requires a frame copy and is not what I went through all the >> >>> effort for. >> >>> >> >>> Why didn't you say anything when the kmsgrab code did the same thing? >> >>> Or when the unwrapped frame stuff was added in the first place? >> >> >> >> I did the other day on IRC when you asked me why i was against this, if >> >> you recall, because it was then when i found out this has been the case >> >> for a long while, and why I'm now saying adding even more cases is >> >> going >> >> in the opposite direction of an actual solution. >> >> >> >> In any case, i explicitly didn't block this, and no one else seems to >> >> care, so whatever. >> > >> > I just now realized this, no, use of sizeof(AVFrame) outside libavutil >> > is not ok, >> > thats a ABI/API breakage. >> > >> > This must be removed/reverted. In all cases that are relevant. >> >> Fixed/adapted, not reverted. For vapoursynth and kmsgrab it should be a >> matter of making them output rawvideo packets. > > Why don't you get it? We don't want to copy and then end up with > unaligned shit. The days of micro optimizations are passed?
On 5/5/2018 5:33 AM, wm4 wrote: > On Fri, 4 May 2018 21:51:38 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 5/4/2018 9:19 PM, Michael Niedermayer wrote: >>> On Fri, May 04, 2018 at 02:02:02PM -0300, James Almer wrote: >>>> On 5/4/2018 1:51 PM, wm4 wrote: >>>>> On Fri, 4 May 2018 13:30:38 -0300 >>>>> James Almer <jamrial@gmail.com> wrote: >>>>> >>>>>> On 5/4/2018 12:58 PM, wm4 wrote: >>>>>>> On Sat, 28 Apr 2018 19:05:29 +0200 >>>>>>> wm4 <nfxjfg@googlemail.com> wrote: >>>>>>> >>>>>>>> This can "demux" .vpy files. >>>>>>>> >>>>>>>> Some minor code copied from other LGPL parts of FFmpeg. >>>>>>>> >>>>>>>> I did not found a good way to test a few of the more obscure features, >>>>>>>> like VFR nodes, compat pixel formats, or nodes with dynamic size/format >>>>>>>> changes. These can be easily implemented on demand. >>>>>>>> --- >>>>>>>> configure | 5 + >>>>>>>> libavformat/Makefile | 1 + >>>>>>>> libavformat/allformats.c | 1 + >>>>>>>> libavformat/vapoursynth.c | 421 ++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 4 files changed, 428 insertions(+) >>>>>>>> create mode 100644 libavformat/vapoursynth.c >>>>>>>> >>>>>>> >>>>>>> Pushed, with some minor changes, and zero-copy frame passing. >>>>>> >>>>>> The first step to fix something (in this case, usage sizeof(AVFrame) >>>>>> outside libavutil) is not adding even more cases of the issue in question. >>>>>> You still can replace this with rawvideo. Someone even already wrote it >>>>>> for you. >>>>>> >>>>>> Lets try to abide our own ABI rules... >>>>> >>>>> That's requires a frame copy and is not what I went through all the >>>>> effort for. >>>>> >>>>> Why didn't you say anything when the kmsgrab code did the same thing? >>>>> Or when the unwrapped frame stuff was added in the first place? >>>> >>>> I did the other day on IRC when you asked me why i was against this, if >>>> you recall, because it was then when i found out this has been the case >>>> for a long while, and why I'm now saying adding even more cases is going >>>> in the opposite direction of an actual solution. >>>> >>>> In any case, i explicitly didn't block this, and no one else seems to >>>> care, so whatever. >>> >>> I just now realized this, no, use of sizeof(AVFrame) outside libavutil is not ok, >>> thats a ABI/API breakage. >>> >>> This must be removed/reverted. In all cases that are relevant. >> >> Fixed/adapted, not reverted. For vapoursynth and kmsgrab it should be a >> matter of making them output rawvideo packets. > > Why don't you get it? We don't want to copy and then end up with > unaligned shit. Has this been a problem with Avisynth? And you're telling that to me, who spent the past couple months dicking around with buffer references to remove as many allocations and copies i could find. I'm all about zero copy, but not when it means an ABI violation. Back to the subject in question, you suggested a function to return sizeof(AVFrame) at runtime. It could even be avpriv to prevent its usage outside of libav*, so i guess that's a good solution to avoid having to rewrite a bunch of modules.
diff --git a/configure b/configure index 9fa1665496..fcc93ecc30 100755 --- a/configure +++ b/configure @@ -303,6 +303,7 @@ External library support: --disable-sdl2 disable sdl2 [autodetect] --disable-securetransport disable Secure Transport, needed for TLS support on OSX if openssl and gnutls are not used [autodetect] + --enable-vapoursynth enable VapourSynth demuxer [no] --disable-xlib disable xlib [autodetect] --disable-zlib disable zlib [autodetect] @@ -1724,6 +1725,7 @@ EXTERNAL_LIBRARY_LIST=" mediacodec openal opengl + vapoursynth " HWACCEL_AUTODETECT_LIBRARY_LIST=" @@ -3088,6 +3090,7 @@ libx265_encoder_deps="libx265" libxavs_encoder_deps="libxavs" libxvid_encoder_deps="libxvid" libzvbi_teletext_decoder_deps="libzvbi" +vapoursynth_demuxer_deps="vapoursynth" videotoolbox_suggest="coreservices" videotoolbox_deps="corefoundation coremedia corevideo" videotoolbox_encoder_deps="videotoolbox VTCompressionSessionPrepareToEncodeFrames" @@ -6130,6 +6133,8 @@ enabled rkmpp && { require_pkg_config rkmpp rockchip_mpp rockchip/r { enabled libdrm || die "ERROR: rkmpp requires --enable-libdrm"; } } +enabled vapoursynth && require_pkg_config vapoursynth "vapoursynth-script >= 42" VSScript.h vsscript_init + if enabled gcrypt; then GCRYPT_CONFIG="${cross_prefix}libgcrypt-config" diff --git a/libavformat/Makefile b/libavformat/Makefile index 3eeca5091d..e1e74a8f43 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -570,6 +570,7 @@ OBJS-$(CONFIG_LIBRTMPTE_PROTOCOL) += librtmp.o OBJS-$(CONFIG_LIBSRT_PROTOCOL) += libsrt.o OBJS-$(CONFIG_LIBSSH_PROTOCOL) += libssh.o OBJS-$(CONFIG_LIBSMBCLIENT_PROTOCOL) += libsmbclient.o +OBJS-$(CONFIG_VAPOURSYNTH_DEMUXER) += vapoursynth.o # protocols I/O OBJS-$(CONFIG_ASYNC_PROTOCOL) += async.o diff --git a/libavformat/allformats.c b/libavformat/allformats.c index d582778b3b..a94364f41d 100644 --- a/libavformat/allformats.c +++ b/libavformat/allformats.c @@ -482,6 +482,7 @@ extern AVOutputFormat ff_chromaprint_muxer; extern AVInputFormat ff_libgme_demuxer; extern AVInputFormat ff_libmodplug_demuxer; extern AVInputFormat ff_libopenmpt_demuxer; +extern AVInputFormat ff_vapoursynth_demuxer; #include "libavformat/muxer_list.c" #include "libavformat/demuxer_list.c" diff --git a/libavformat/vapoursynth.c b/libavformat/vapoursynth.c new file mode 100644 index 0000000000..6758a7bf31 --- /dev/null +++ b/libavformat/vapoursynth.c @@ -0,0 +1,421 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** +* @file +* VapourSynth demuxer +* +* Synthesizes vapour (?) +*/ + +#include <limits.h> + +#include <VapourSynth.h> +#include <VSScript.h> + +#include "libavutil/avassert.h" +#include "libavutil/avstring.h" +#include "libavutil/eval.h" +#include "libavutil/imgutils.h" +#include "libavutil/opt.h" +#include "libavutil/pixdesc.h" +#include "avformat.h" +#include "internal.h" + +typedef struct VSContext { + const AVClass *class; + + const VSAPI *vsapi; + VSCore *vscore; + VSScript *vss; + int did_init; + + VSNodeRef *outnode; + int is_cfr; + int current_frame; + + int c_order[4]; + + /* options */ + int64_t max_size; +} VSContext; + +#define OFFSET(x) offsetof(VSContext, x) +#define A AV_OPT_FLAG_AUDIO_PARAM +#define D AV_OPT_FLAG_DECODING_PARAM +static const AVOption options[] = { + {"max_size", "set max file size supported (in bytes)", OFFSET(max_size), AV_OPT_TYPE_INT64, {.i64 = 1 * 1024 * 1024}, 0, SIZE_MAX - 1, A|D}, + {NULL} +}; + +static av_cold int read_close_vs(AVFormatContext *s) +{ + VSContext *vs = s->priv_data; + + if (vs->outnode) + vs->vsapi->freeNode(vs->outnode); + + vsscript_freeScript(vs->vss); + vs->vss = NULL; + vs->vsapi = NULL; + vs->vscore = NULL; + vs->outnode = NULL; + + if (vs->did_init) + vsscript_finalize(); + vs->did_init = 0; + + return 0; +} + +static av_cold int is_native_endian(enum AVPixelFormat pixfmt) +{ + enum AVPixelFormat other = av_pix_fmt_swap_endianness(pixfmt); + const AVPixFmtDescriptor *pd; + if (other == AV_PIX_FMT_NONE || other == pixfmt) + return 1; // not affected by byte order + pd = av_pix_fmt_desc_get(pixfmt); + return pd && (!!HAVE_BIGENDIAN == !!(pd->flags & AV_PIX_FMT_FLAG_BE)); +} + +static av_cold enum AVPixelFormat match_pixfmt(const VSFormat *vsf, int c_order[4]) +{ + static const int yuv_order[4] = {0, 1, 2, 0}; + static const int rgb_order[4] = {1, 2, 0, 0}; + const AVPixFmtDescriptor *pd; + + for (pd = av_pix_fmt_desc_next(NULL); pd; pd = av_pix_fmt_desc_next(pd)) { + int is_rgb, is_yuv, i; + const int *order; + enum AVPixelFormat pixfmt; + + pixfmt = av_pix_fmt_desc_get_id(pd); + + if (pd->flags & (AV_PIX_FMT_FLAG_BAYER | AV_PIX_FMT_FLAG_ALPHA | + AV_PIX_FMT_FLAG_HWACCEL | AV_PIX_FMT_FLAG_BITSTREAM)) + continue; + + if (pd->log2_chroma_w != vsf->subSamplingW || + pd->log2_chroma_h != vsf->subSamplingH) + continue; + + is_rgb = vsf->colorFamily == cmRGB; + if (is_rgb != !!(pd->flags & AV_PIX_FMT_FLAG_RGB)) + continue; + + is_yuv = vsf->colorFamily == cmYUV || + vsf->colorFamily == cmYCoCg || + vsf->colorFamily == cmGray; + if (!is_rgb && !is_yuv) + continue; + + if (vsf->sampleType != ((pd->flags & AV_PIX_FMT_FLAG_FLOAT) ? stFloat : stInteger)) + continue; + + if (av_pix_fmt_count_planes(pixfmt) != vsf->numPlanes) + continue; + + if (strncmp(pd->name, "xyz", 3) == 0) + continue; + + if (!is_native_endian(pixfmt)) + continue; + + order = is_yuv ? yuv_order : rgb_order; + + for (i = 0; i < pd->nb_components; i++) { + const AVComponentDescriptor *c = &pd->comp[i]; + if (order[c->plane] != i || + c->offset != 0 || c->shift != 0 || + c->step != vsf->bytesPerSample || + c->depth != vsf->bitsPerSample) + goto cont; + } + + // Use it. + memcpy(c_order, order, sizeof(int[4])); + return pixfmt; + + cont: ; + } + + return AV_PIX_FMT_NONE; +} + +static av_cold int read_header_vs(AVFormatContext *s) +{ + AVStream *st; + AVIOContext *pb = s->pb; + VSContext *vs = s->priv_data; + int64_t sz = avio_size(pb); + char *buf = NULL; + char dummy; + const VSVideoInfo *info; + int err; + + if (!vsscript_init()) { + av_log(s, AV_LOG_ERROR, "Failed to initialize VSScript (possibly PYTHONPATH not set).\n"); + err = AVERROR_EXTERNAL; + goto done; + } + + vs->did_init = 1; + + if (sz < 0 || sz > vs->max_size) { + if (sz < 0) + av_log(s, AV_LOG_WARNING, "Could not determine file size\n"); + sz = vs->max_size; + } + + buf = av_malloc(sz + 1); + if (!buf) { + err = AVERROR(ENOMEM); + goto done; + } + sz = avio_read(pb, buf, sz); + + if (sz < 0) { + av_log(s, AV_LOG_ERROR, "Could not read script.\n"); + err = sz; + goto done; + } + + // Data left means our buffer (the max_size option) is too small + if (avio_read(pb, &dummy, 1) == 1) { + av_log(s, AV_LOG_ERROR, "File size is larger than max_size option " + "value %"PRIi64", consider increasing the max_size option\n", + vs->max_size); + err = AVERROR_BUFFER_TOO_SMALL; + goto done; + } + + if (vsscript_createScript(&vs->vss)) { + av_log(s, AV_LOG_ERROR, "Failed to create script instance.\n"); + err = AVERROR_EXTERNAL; + goto done; + } + + buf[sz] = '\0'; + if (vsscript_evaluateScript(&vs->vss, buf, s->url, 0)) { + const char *msg = vsscript_getError(vs->vss); + av_log(s, AV_LOG_ERROR, "Failed to parse script: %s\n", msg ? msg : "(unknown)"); + err = AVERROR_EXTERNAL; + goto done; + } + + vs->vsapi = vsscript_getVSApi(); + vs->vscore = vsscript_getCore(vs->vss); + + vs->outnode = vsscript_getOutput(vs->vss, 0); + if (!vs->outnode) { + av_log(s, AV_LOG_ERROR, "Could not get script output node.\n"); + err = AVERROR_EXTERNAL; + goto done; + } + + st = avformat_new_stream(s, NULL); + if (!st) { + err = AVERROR(ENOMEM); + goto done; + } + + info = vs->vsapi->getVideoInfo(vs->outnode); + + if (!info->format || !info->width || !info->height) { + av_log(s, AV_LOG_ERROR, "Non-constant input format not supported.\n"); + err = AVERROR_PATCHWELCOME; + goto done; + } + + if (info->fpsDen) { + vs->is_cfr = 1; + avpriv_set_pts_info(st, 64, info->fpsDen, info->fpsNum); + st->duration = info->numFrames; + } else { + // VFR. Just set "something". + avpriv_set_pts_info(st, 64, 1, AV_TIME_BASE); + s->ctx_flags |= AVFMTCTX_UNSEEKABLE; + } + + st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO; + st->codecpar->codec_id = AV_CODEC_ID_WRAPPED_AVFRAME; + st->codecpar->width = info->width; + st->codecpar->height = info->height; + st->codecpar->format = match_pixfmt(info->format, vs->c_order); + + if (st->codecpar->format == AV_PIX_FMT_NONE) { + av_log(s, AV_LOG_ERROR, "Unsupported VS pixel format %s\n", info->format->name); + err = AVERROR_EXTERNAL; + goto done; + } + av_log(s, AV_LOG_VERBOSE, "VS format %s -> pixfmt %s\n", info->format->name, + av_get_pix_fmt_name(st->codecpar->format)); + + if (info->format->colorFamily == cmYCoCg) + st->codecpar->color_space = AVCOL_SPC_YCGCO; + +done: + av_free(buf); + if (err < 0) + read_close_vs(s); + return err; +} + +static void free_frame(void *opaque, uint8_t *data) +{ + AVFrame *frame = (AVFrame *)data; + + av_frame_free(&frame); +} + +static int get_vs_prop_int(AVFormatContext *s, const VSMap *map, const char *name, int def) +{ + VSContext *vs = s->priv_data; + int64_t res; + int err = 1; + + res = vs->vsapi->propGetInt(map, name, 0, &err); + return err || res < INT_MIN || res > INT_MAX ? def : res; +} + +static int read_packet_vs(AVFormatContext *s, AVPacket *pkt) +{ + VSContext *vs = s->priv_data; + AVStream *st = s->streams[0]; + AVFrame *frame = NULL; + char vserr[80]; + const VSFrameRef *vsframe = NULL; + const VSVideoInfo *info = vs->vsapi->getVideoInfo(vs->outnode); + int err = 0; + const uint8_t *src_data[4]; + int src_linesizes[4]; + const VSMap *props; + int i; + + if (vs->current_frame >= info->numFrames) + return AVERROR_EOF; + + vsframe = vs->vsapi->getFrame(vs->current_frame, vs->outnode, vserr, sizeof(vserr)); + if (!vsframe) { + av_log(s, AV_LOG_ERROR, "Error getting frame: %s\n", vserr); + err = AVERROR_EXTERNAL; + goto end; + } + + props = vs->vsapi->getFramePropsRO(vsframe); + + frame = av_frame_alloc(); + if (!frame) { + err = AVERROR(ENOMEM); + goto end; + } + + frame->format = st->codecpar->format; + frame->width = st->codecpar->width; + frame->height = st->codecpar->height; + frame->colorspace = st->codecpar->color_space; + + // Values according to ISO/IEC 14496-10. + frame->colorspace = get_vs_prop_int(s, props, "_Matrix", frame->colorspace); + frame->color_primaries = get_vs_prop_int(s, props, "_Primaries", frame->color_primaries); + frame->color_trc = get_vs_prop_int(s, props, "_Transfer", frame->color_trc); + + if (get_vs_prop_int(s, props, "_ColorRange", 1) == 0) + frame->color_range = AVCOL_RANGE_JPEG; + + frame->sample_aspect_ratio.num = get_vs_prop_int(s, props, "_SARNum", 0); + frame->sample_aspect_ratio.den = get_vs_prop_int(s, props, "_SARDen", 1); + + av_assert0(vs->vsapi->getFrameWidth(vsframe, 0) == frame->width); + av_assert0(vs->vsapi->getFrameHeight(vsframe, 0) == frame->height); + + err = av_frame_get_buffer(frame, 0); + if (err < 0) + goto end; + + for (i = 0; i < info->format->numPlanes; i++) { + int p = vs->c_order[i]; + src_data[i] = vs->vsapi->getReadPtr(vsframe, p); + src_linesizes[i] = vs->vsapi->getStride(vsframe, p); + } + + av_image_copy(frame->data, frame->linesize, src_data, src_linesizes, + frame->format, frame->width, frame->height); + + pkt->buf = av_buffer_create((uint8_t*)frame, sizeof(*frame), + free_frame, NULL, 0); + if (!pkt->buf) { + err = AVERROR(ENOMEM); + goto end; + } + + frame = NULL; // pkt owns it now + + pkt->data = pkt->buf->data; + pkt->size = pkt->buf->size; + pkt->flags |= AV_PKT_FLAG_TRUSTED; + + if (vs->is_cfr) + pkt->pts = vs->current_frame; + + vs->current_frame++; + +end: + if (err < 0) + av_packet_unref(pkt); + av_frame_free(&frame); + vs->vsapi->freeFrame(vsframe); + return err; +} + +static int read_seek_vs(AVFormatContext *s, int stream_idx, int64_t ts, int flags) +{ + VSContext *vs = s->priv_data; + + if (!vs->is_cfr) + return AVERROR(ENOSYS); + + vs->current_frame = FFMIN(FFMAX(0, ts), s->streams[0]->duration); + return 0; +} + +static av_cold int probe_vs(AVProbeData *p) +{ + // Explicitly do not support this. VS scripts are written in Python, and + // can run arbitrary code on the user's system. + return 0; +} + +static const AVClass class_vs = { + .class_name = "VapourSynth demuxer", + .item_name = av_default_item_name, + .option = options, + .version = LIBAVUTIL_VERSION_INT, +}; + +AVInputFormat ff_vapoursynth_demuxer = { + .name = "vapoursynth", + .long_name = NULL_IF_CONFIG_SMALL("VapourSynth demuxer"), + .priv_data_size = sizeof(VSContext), + .read_probe = probe_vs, + .read_header = read_header_vs, + .read_packet = read_packet_vs, + .read_close = read_close_vs, + .read_seek = read_seek_vs, + .priv_class = &class_vs, +};