On Sat, Oct 26, 2013 at 11:04 AM, Vittorio Giovara <[email protected]> wrote: > Hi, > > On Thu, Sep 12, 2013 at 10:09 PM, Stephen Hutchinson <[email protected]> wrote: >> On Thu, Sep 12, 2013 at 4:59 AM, Diego Biurrun <[email protected]> wrote: >> >>> On Wed, Sep 11, 2013 at 10:14:07PM -0400, Stephen Hutchinson wrote: >>> > From: d s <[email protected]> >>> >>> I repeat: Can we please have a name to go along with this? >>> >> >> I've shot off an email to them. > > Did you get any reply back?
Nope. I've not gotten any reply. >>> > --- a/libavformat/avisynth.c >>> > +++ b/libavformat/avisynth.c >>> > @@ -19,211 +19,672 @@ >>> > + >>> > +// AvxSynth doesn't have these colorspaces, so disable them >>> > +#ifndef _WIN32 >>> > +#define avs_is_yv24(vi) 0 >>> > +#define avs_is_yv16(vi) 0 >>> > +#define avs_is_yv411(vi) 0 >>> > +#define avs_is_y8(vi) 0 >>> > +#endif >>> >>> The comment contradicts the ifdef condition. How is the support of >>> colorspaces related to the operating system? >>> >> >> The support of those four colorspaces is conditional on whether AviSynth or >> AvxSynth is being used, and the divide between the two is where the OS >> comes in. If Windows is not defined (and therefore AvxSynth is being >> used), those get forcibly disabled. > > I have never used AvxSynth, but if one day someone gets to run it on > windows, this define will trip them. > Is there no symbol or macro in either Avisynth or Avxsynth headers > that we could use instead? > > If not then, I think I'd be nice if you wrote this in the comment, so > that more information about this peculiar ifdef is kept around. I don't know of any symbol or macro that would do that, no (it's actually just that those four functions simply don't exist in AvxSynth while they do in AviSynth 2.6). I could try to use the same method used to stop AviSynth 2.5 from loading, but even if AvxSynth eventually gets running on Windows, the demuxer as it stands would ignore it because the DLL name would be different and on Windows, it only ever looks for avisynth itself. It's also extremely unlikely that AvxSynth will ever make the jump to Windows, because it'd be redundant against AviSynth 2.5 (the support for which was already removed from this patch). If anything it's probably more likely for AvxSynth to come into feature parity with AviSynth 2.6, which would allow it to support those colorspaces. With its development pace over the the last six months, though, I doubt this happening any time soon. But even regardless of that possibility, it's looking far more likely for AviSynth+ (the new fork that coalesced a little over a month ago; http://forum.doom9.org/showthread.php?t=168856) to jump to cross-platform, which would eclipse AvxSynth entirely and also make that block completely unnecessary. By 'far more likely' I mean, cross-platform support is actually a planned feature on the roadmap. I don't want to make any premature predictions, but it'll probably also eclipse classic AviSynth if the current development pace is any indication (to say nothing of the features it actually plans to implement). I have no idea when the cross-platform support in avsplus might get there, though. So until that happens, AvxSynth is what gets used to cover OSX and Linux (especially as they were also the ones that originally authored this patch). All that considered, I could also just make the comment more verbose about AvxSynth's dim prospects of ever being run on Windows. Or I could nest the if/else that uses those functions in another if/else that gets used only if it detects AviSynth 2.6. I'd have to test. >>> > +static int avisynth_read_packet(AVFormatContext *s, AVPacket *pkt) >>> > +{ >>> > + AviSynthContext *avs = s->priv_data; >>> > + AVStream *st; >>> > + int discard = 0; >>> > + int ret; >>> > + >>> > + if (avs->error) >>> > + return AVERROR_UNKNOWN; >>> > + >>> > + pkt->destruct = av_destruct_packet; >>> > + >>> > + // 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; // so that video-only scripts don't hang at end of >>> read >>> > + } 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; // so that audio-only scripts don't hang at end of >>> read >>> > + } >>> > +} >>> >>> You can factor out the return. >>> >> >> As mentioned above and noted in the comments, removing the return ret; >> lines and replacing them with a single return 0; at the end of the function >> cause video-only and audio-only scripts to not exit at the end of being >> read, forcing the user to have to Ctrl+C. It only happens when the script >> serves one and not the other - if both video and audio are present, the >> behavior doesn't occur and the process exits properly. > > Ok, then I'd suggest you mention this more verbosely in the comment. Will do. > Besides that I'd say the patch is good to go. > So can you please send an updated version? Sure. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
