On Mon, Oct 28, 2013 at 6:16 AM, Diego Biurrun <[email protected]> wrote:
> On Thu, Sep 12, 2013 at 04:09:52PM -0400, Stephen Hutchinson wrote:
>> I've shot off an email to them.
>
> Tried again?
I pinged them on Github this morning, since there was a commit pushed
to AvxSynth in the last few days. Maybe the mail got lost, but on
Github it should show up in the notifications. Still don't know if
I'll get any response back.
>> > > --- 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.
The next version of the patch gets rid of that block entirely. It's
an artifact from x264's AvxSynth support patch, and so there's now a
USING_AVISYNTH macro that gets set only when AviSynth is used. All
but the first of the _WIN32 ifdefs are now USING_AVISYNTH instead,
since the two are synonymous, and the bits-per-pixel if/else that the
block was originally there for (to stop implicit declaration errors
when using AvxSynth) is now activated only when AviSynth is used.
>> > > +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.
The issue I stumbled across was that I'd used return 0, since I
wrongly assumed it wouldn't see the value of ret outside of the
if/else block. That's what underlaid the hang. Fixed.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
