On Thu, Sep 12, 2013 at 04:09:52PM -0400, Stephen Hutchinson 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.
Tried again?
> > > --- 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.
You are making an indirect argument here, which shows how brittle the
assumption is. The condition should check which of the two libs is
used, or - better yet - check if the colorspaces are actually supported.
> > > +#define LOAD_AVS_FUNC(name, continue_on_fail) \
> > > + { \
> > > + avs_library->name = \
> > > + (void *)GetProcAddress(avs_library->library, #name); \
> > > + if (!continue_on_fail && !avs_library->name) \
> > > + goto fail; \
> > > + }
> > > + LOAD_AVS_FUNC(avs_bit_blt, 0);
> > > + LOAD_AVS_FUNC(avs_clip_get_error, 0);
> >
> > The extra {} look unnecessary.
>
> Aren't those necessary so that the entire {} block is defined as the
> meaning of LOAD_AVS_FUNC(name, continue_on_fail)?
No, the backslashes take care of that.
> > > +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.
I never said you should remove the return. I said you should factor it out.
Just move it after the if-else block instead of having the same line inside
of both branches.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
