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?
>
>> > --- 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.
>
>
>> > +#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, they would be necessary only if LOAD_AVS_FUNC() were used outside
any function body.
It seems you're using it only inside avisynth_load_library so it's
safe to remove the {}.
>
>
>> > +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.
Besides that I'd say the patch is good to go.
So can you please send an updated version?
Vittorio
> _______________________________________________
> libav-devel mailing list
> [email protected]
> https://lists.libav.org/mailman/listinfo/libav-devel
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
