[FFmpeg-devel] avfilter/vf_stereo3d: implement auto detection by using frame side data

Submitted by Paul B Mahol on Nov. 30, 2017, 10:11 p.m.

Details

Message ID 20171130221146.15471-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Nov. 30, 2017, 10:11 p.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 doc/filters.texi          |  4 ++++
 libavfilter/avfilter.c    | 30 ++++++++++++++++++++++++++++++
 libavfilter/internal.h    |  2 ++
 libavfilter/vf_stereo3d.c | 37 ++++++++++++++++++++++++++++++++++---
 4 files changed, 70 insertions(+), 3 deletions(-)

Comments

Nicolas George Dec. 1, 2017, 9:01 a.m.
Paul B Mahol (2017-11-30):
> +static int reset_links(AVFilterContext *filter)
> +{
> +    int i, ret;
> +
> +    if (!filter)
> +        return 0;
> +
> +    for (i = 0; i < filter->nb_outputs; i++) {
> +        AVFilterLink *link = filter->outputs[i];
> +
> +        link->init_state = AVLINK_UNINIT;
> +        link->frame_rate.num = link->frame_rate.den = 0;
> +        link->w = link->h = 0;
> +
> +        ret = reset_links(link->dst);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    if (!i)
> +        return avfilter_config_links(filter);
> +
> +    return 0;
> +}

Sorry, but no. All filters are currently written with the assumption
that config_props is called only once. Not all of them actually rely on
this assumption, but some do, and expect the fields of the private
context to be in their initial state. Violating that assumption would
result in a lot of memory leaks and probably a few crashes, some of them
security relevant.

Plus, the public API of buffersink does not document params changes and
does not provide a good way of reacting to it, so enabling params
changes like that would have problematic results on applications that
expect frames with all the same size.

(By the way, I am quite worried to see that Michael weakened the
protections for that last point in 5d859e59809.)

Filters reconfiguration has been wanted for a lot of time. If it was
that simple, it would have been done already. If you want to implement
it, please go ahead, but it will not be an easy task.

Regards,
Paul B Mahol Dec. 1, 2017, 9:35 a.m.
On 12/1/17, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (2017-11-30):
>> +static int reset_links(AVFilterContext *filter)
>> +{
>> +    int i, ret;
>> +
>> +    if (!filter)
>> +        return 0;
>> +
>> +    for (i = 0; i < filter->nb_outputs; i++) {
>> +        AVFilterLink *link = filter->outputs[i];
>> +
>> +        link->init_state = AVLINK_UNINIT;
>> +        link->frame_rate.num = link->frame_rate.den = 0;
>> +        link->w = link->h = 0;
>> +
>> +        ret = reset_links(link->dst);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    if (!i)
>> +        return avfilter_config_links(filter);
>> +
>> +    return 0;
>> +}
>
> Sorry, but no. All filters are currently written with the assumption
> that config_props is called only once. Not all of them actually rely on
> this assumption, but some do, and expect the fields of the private
> context to be in their initial state. Violating that assumption would
> result in a lot of memory leaks and probably a few crashes, some of them
> security relevant.

Not all filters are written with such assumption. And I would like to fix those
who rely on this assumption.

>
> Plus, the public API of buffersink does not document params changes and
> does not provide a good way of reacting to it, so enabling params
> changes like that would have problematic results on applications that
> expect frames with all the same size.

Well for that point one can always autoinsert scale filter if user does
not set some flag when calling buffersink function.

>
> (By the way, I am quite worried to see that Michael weakened the
> protections for that last point in 5d859e59809.)
>
> Filters reconfiguration has been wanted for a lot of time. If it was
> that simple, it would have been done already. If you want to implement
> it, please go ahead, but it will not be an easy task.

I would like to proceed but I can't guess what is in your mind.
wm4 Dec. 1, 2017, 4:23 p.m.
On Fri, 1 Dec 2017 10:35:34 +0100
Paul B Mahol <onemda@gmail.com> wrote:

> On 12/1/17, Nicolas George <george@nsup.org> wrote:
> > Paul B Mahol (2017-11-30):  
> >> +static int reset_links(AVFilterContext *filter)
> >> +{
> >> +    int i, ret;
> >> +
> >> +    if (!filter)
> >> +        return 0;
> >> +
> >> +    for (i = 0; i < filter->nb_outputs; i++) {
> >> +        AVFilterLink *link = filter->outputs[i];
> >> +
> >> +        link->init_state = AVLINK_UNINIT;
> >> +        link->frame_rate.num = link->frame_rate.den = 0;
> >> +        link->w = link->h = 0;
> >> +
> >> +        ret = reset_links(link->dst);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +    }
> >> +
> >> +    if (!i)
> >> +        return avfilter_config_links(filter);
> >> +
> >> +    return 0;
> >> +}  
> >
> > Sorry, but no. All filters are currently written with the assumption
> > that config_props is called only once. Not all of them actually rely on
> > this assumption, but some do, and expect the fields of the private
> > context to be in their initial state. Violating that assumption would
> > result in a lot of memory leaks and probably a few crashes, some of them
> > security relevant.  
> 
> Not all filters are written with such assumption. And I would like to fix those
> who rely on this assumption.

SGTM

> >
> > Plus, the public API of buffersink does not document params changes and
> > does not provide a good way of reacting to it, so enabling params
> > changes like that would have problematic results on applications that
> > expect frames with all the same size.  
> 
> Well for that point one can always autoinsert scale filter if user does
> not set some flag when calling buffersink function.

Also SGTM.
Michael Niedermayer Dec. 1, 2017, 5:02 p.m.
On Fri, Dec 01, 2017 at 10:01:42AM +0100, Nicolas George wrote:
> Paul B Mahol (2017-11-30):
> > +static int reset_links(AVFilterContext *filter)
> > +{
> > +    int i, ret;
> > +
> > +    if (!filter)
> > +        return 0;
> > +
> > +    for (i = 0; i < filter->nb_outputs; i++) {
> > +        AVFilterLink *link = filter->outputs[i];
> > +
> > +        link->init_state = AVLINK_UNINIT;
> > +        link->frame_rate.num = link->frame_rate.den = 0;
> > +        link->w = link->h = 0;
> > +
> > +        ret = reset_links(link->dst);
> > +        if (ret < 0)
> > +            return ret;
> > +    }
> > +
> > +    if (!i)
> > +        return avfilter_config_links(filter);
> > +
> > +    return 0;
> > +}
> 
> Sorry, but no. All filters are currently written with the assumption
> that config_props is called only once. Not all of them actually rely on
> this assumption, but some do, and expect the fields of the private
> context to be in their initial state. Violating that assumption would
> result in a lot of memory leaks and probably a few crashes, some of them
> security relevant.
> 
> Plus, the public API of buffersink does not document params changes and
> does not provide a good way of reacting to it, so enabling params
> changes like that would have problematic results on applications that
> expect frames with all the same size.
> 
> (By the way, I am quite worried to see that Michael weakened the
> protections for that last point in 5d859e59809.)
> 

> Filters reconfiguration has been wanted for a lot of time. If it was
> that simple, it would have been done already. If you want to implement
> it, please go ahead, but it will not be an easy task.

As the one who tried implementing changing frame parameters like
dimension long time ago, the only real problem i encountered was
bikeshedding, i dont rememer a technical problem. The bikeshedding in
fact resolved itself but by the time i had lost interrest ...

And in fact many filters should just be able to handle changing frame
parameters as they are or with minimal changes.

[...]
wm4 Dec. 1, 2017, 5:10 p.m.
On Fri, 1 Dec 2017 18:02:52 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Fri, Dec 01, 2017 at 10:01:42AM +0100, Nicolas George wrote:
> > Paul B Mahol (2017-11-30):  
> > > +static int reset_links(AVFilterContext *filter)
> > > +{
> > > +    int i, ret;
> > > +
> > > +    if (!filter)
> > > +        return 0;
> > > +
> > > +    for (i = 0; i < filter->nb_outputs; i++) {
> > > +        AVFilterLink *link = filter->outputs[i];
> > > +
> > > +        link->init_state = AVLINK_UNINIT;
> > > +        link->frame_rate.num = link->frame_rate.den = 0;
> > > +        link->w = link->h = 0;
> > > +
> > > +        ret = reset_links(link->dst);
> > > +        if (ret < 0)
> > > +            return ret;
> > > +    }
> > > +
> > > +    if (!i)
> > > +        return avfilter_config_links(filter);
> > > +
> > > +    return 0;
> > > +}  
> > 
> > Sorry, but no. All filters are currently written with the assumption
> > that config_props is called only once. Not all of them actually rely on
> > this assumption, but some do, and expect the fields of the private
> > context to be in their initial state. Violating that assumption would
> > result in a lot of memory leaks and probably a few crashes, some of them
> > security relevant.
> > 
> > Plus, the public API of buffersink does not document params changes and
> > does not provide a good way of reacting to it, so enabling params
> > changes like that would have problematic results on applications that
> > expect frames with all the same size.
> > 
> > (By the way, I am quite worried to see that Michael weakened the
> > protections for that last point in 5d859e59809.)
> >   
> 
> > Filters reconfiguration has been wanted for a lot of time. If it was
> > that simple, it would have been done already. If you want to implement
> > it, please go ahead, but it will not be an easy task.  
> 
> As the one who tried implementing changing frame parameters like
> dimension long time ago, the only real problem i encountered was
> bikeshedding, i dont rememer a technical problem. The bikeshedding in
> fact resolved itself but by the time i had lost interrest ...
> 
> And in fact many filters should just be able to handle changing frame
> parameters as they are or with minimal changes.

Well, the result is that at least the vf_scale filter appears to
blatantly violate the API and crashes my software.
Paul B Mahol Dec. 1, 2017, 5:16 p.m.
On 12/1/17, wm4 <nfxjfg@googlemail.com> wrote:
> On Fri, 1 Dec 2017 18:02:52 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
>
>> On Fri, Dec 01, 2017 at 10:01:42AM +0100, Nicolas George wrote:
>> > Paul B Mahol (2017-11-30):
>> > > +static int reset_links(AVFilterContext *filter)
>> > > +{
>> > > +    int i, ret;
>> > > +
>> > > +    if (!filter)
>> > > +        return 0;
>> > > +
>> > > +    for (i = 0; i < filter->nb_outputs; i++) {
>> > > +        AVFilterLink *link = filter->outputs[i];
>> > > +
>> > > +        link->init_state = AVLINK_UNINIT;
>> > > +        link->frame_rate.num = link->frame_rate.den = 0;
>> > > +        link->w = link->h = 0;
>> > > +
>> > > +        ret = reset_links(link->dst);
>> > > +        if (ret < 0)
>> > > +            return ret;
>> > > +    }
>> > > +
>> > > +    if (!i)
>> > > +        return avfilter_config_links(filter);
>> > > +
>> > > +    return 0;
>> > > +}
>> >
>> > Sorry, but no. All filters are currently written with the assumption
>> > that config_props is called only once. Not all of them actually rely on
>> > this assumption, but some do, and expect the fields of the private
>> > context to be in their initial state. Violating that assumption would
>> > result in a lot of memory leaks and probably a few crashes, some of them
>> > security relevant.
>> >
>> > Plus, the public API of buffersink does not document params changes and
>> > does not provide a good way of reacting to it, so enabling params
>> > changes like that would have problematic results on applications that
>> > expect frames with all the same size.
>> >
>> > (By the way, I am quite worried to see that Michael weakened the
>> > protections for that last point in 5d859e59809.)
>> >
>>
>> > Filters reconfiguration has been wanted for a lot of time. If it was
>> > that simple, it would have been done already. If you want to implement
>> > it, please go ahead, but it will not be an easy task.
>>
>> As the one who tried implementing changing frame parameters like
>> dimension long time ago, the only real problem i encountered was
>> bikeshedding, i dont rememer a technical problem. The bikeshedding in
>> fact resolved itself but by the time i had lost interrest ...
>>
>> And in fact many filters should just be able to handle changing frame
>> parameters as they are or with minimal changes.
>
> Well, the result is that at least the vf_scale filter appears to
> blatantly violate the API and crashes my software.

How?

The current "support" works only with some filters and when input itself
have parameter changes, using other filters in same filtergraph will just
crash.
Michael Niedermayer Dec. 1, 2017, 6:17 p.m.
On Fri, Dec 01, 2017 at 06:10:02PM +0100, wm4 wrote:
> On Fri, 1 Dec 2017 18:02:52 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Fri, Dec 01, 2017 at 10:01:42AM +0100, Nicolas George wrote:
> > > Paul B Mahol (2017-11-30):  
> > > > +static int reset_links(AVFilterContext *filter)
> > > > +{
> > > > +    int i, ret;
> > > > +
> > > > +    if (!filter)
> > > > +        return 0;
> > > > +
> > > > +    for (i = 0; i < filter->nb_outputs; i++) {
> > > > +        AVFilterLink *link = filter->outputs[i];
> > > > +
> > > > +        link->init_state = AVLINK_UNINIT;
> > > > +        link->frame_rate.num = link->frame_rate.den = 0;
> > > > +        link->w = link->h = 0;
> > > > +
> > > > +        ret = reset_links(link->dst);
> > > > +        if (ret < 0)
> > > > +            return ret;
> > > > +    }
> > > > +
> > > > +    if (!i)
> > > > +        return avfilter_config_links(filter);
> > > > +
> > > > +    return 0;
> > > > +}  
> > > 
> > > Sorry, but no. All filters are currently written with the assumption
> > > that config_props is called only once. Not all of them actually rely on
> > > this assumption, but some do, and expect the fields of the private
> > > context to be in their initial state. Violating that assumption would
> > > result in a lot of memory leaks and probably a few crashes, some of them
> > > security relevant.
> > > 
> > > Plus, the public API of buffersink does not document params changes and
> > > does not provide a good way of reacting to it, so enabling params
> > > changes like that would have problematic results on applications that
> > > expect frames with all the same size.
> > > 
> > > (By the way, I am quite worried to see that Michael weakened the
> > > protections for that last point in 5d859e59809.)
> > >   
> > 
> > > Filters reconfiguration has been wanted for a lot of time. If it was
> > > that simple, it would have been done already. If you want to implement
> > > it, please go ahead, but it will not be an easy task.  
> > 
> > As the one who tried implementing changing frame parameters like
> > dimension long time ago, the only real problem i encountered was
> > bikeshedding, i dont rememer a technical problem. The bikeshedding in
> > fact resolved itself but by the time i had lost interrest ...
> > 
> > And in fact many filters should just be able to handle changing frame
> > parameters as they are or with minimal changes.
> 
> Well, the result is that at least the vf_scale filter appears to
> blatantly violate the API and crashes my software.

This sounds unlikely but
Which ticket on trac is that ?
And what part of which API do you speak about ?

[...]
Nicolas George Dec. 1, 2017, 6:23 p.m.
Michael Niedermayer (2017-12-01):
> As the one who tried implementing changing frame parameters like
> dimension long time ago, the only real problem i encountered was
> bikeshedding, i dont rememer a technical problem.

"not easy" != "tremendously hard". I said it would not be easy, i.e. it
will require work and careful testing, but it is doable.

But what you did was wrong, too simple, and results in our libraries
outputting inconsistent data.

No time to say more today.

Regards,
Paul B Mahol Dec. 1, 2017, 6:28 p.m.
On 12/1/17, Nicolas George <george@nsup.org> wrote:
> Michael Niedermayer (2017-12-01):
>> As the one who tried implementing changing frame parameters like
>> dimension long time ago, the only real problem i encountered was
>> bikeshedding, i dont rememer a technical problem.
>
> "not easy" != "tremendously hard". I said it would not be easy, i.e. it
> will require work and careful testing, but it is doable.
>
> But what you did was wrong, too simple, and results in our libraries
> outputting inconsistent data.
>
> No time to say more today.

If you do not have "time" to speak than do not speak at all.
Michael Niedermayer Dec. 1, 2017, 7:08 p.m.
On Fri, Dec 01, 2017 at 07:23:00PM +0100, Nicolas George wrote:
> Michael Niedermayer (2017-12-01):
> > As the one who tried implementing changing frame parameters like
> > dimension long time ago, the only real problem i encountered was
> > bikeshedding, i dont rememer a technical problem.
> 
> "not easy" != "tremendously hard". I said it would not be easy, i.e. it
> will require work and careful testing, but it is doable.
> 

> But what you did was wrong, too simple, and results in our libraries
> outputting inconsistent data.

Do you think this comment will fix any issue or improve anything ?
First i do not even know what you speak of exactly and as this is
supposedly about code i wrote, i doubt others will have a better idea.

And there is possibly a difference in goals. Reinitializing filters
would loose filter state, so avoiding reinit unless really needed was
a big goal. Last but not least what i had envissioned is just in my head,
i never implemeted it. (the bikeshedding made me loose
interrest back then as i already mentioned) what is in git is a small
part of it, that alone certainly is not that great.
and "too simple" yes my vission of this was and is simple


> 
> No time to say more today.
> 
> Regards,
> 
> -- 
>   Nicolas George



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Nicolas George Dec. 3, 2017, 7:21 p.m.
Michael Niedermayer (2017-12-01):
> Do you think this comment will fix any issue or improve anything ?

This comment had one goal: prevent somebody from pushing incorrect
changes before I had time to make my point.

> First i do not even know what you speak of exactly and as this is

I posted the exact reference in my previous mail in this thread, which
is not very long:

https://ffmpeg.org/pipermail/ffmpeg-devel/2017-December/221480.html

> supposedly about code i wrote, i doubt others will have a better idea.
> 
> And there is possibly a difference in goals. Reinitializing filters
> would loose filter state, so avoiding reinit unless really needed was
> a big goal. Last but not least what i had envissioned is just in my head,
> i never implemeted it. (the bikeshedding made me loose
> interrest back then as i already mentioned) what is in git is a small
> part of it, that alone certainly is not that great.
> and "too simple" yes my vission of this was and is simple

I agree that avoiding resets is a worthy goal. I do not know if what you
had in your head was correct or not. But what you committed in
9225513242 (and related commits 5d2b885074, 6c702c3c63, 5d859e5980) was
just wrong. And one of the reason we can be sure it was wrong is that it
caused assert failures, that you did not fix but just hid with more of
the same.

But it is even worse than that: with the last of these commits, the
change can reach buffersink, and therefore the application. The
application will therefore get from buffersink frames with parameters
that do not match the parameters obtained from buffersink itself, it can
be considered a severe bug all by itself. It could even open security
issues for some applications.

What Paul proposes does not have that problem, since it changes the
parameters in buffersink, but it has never been documented that they
could change, and indeed our own source code is written as if they do
not. Therefore, it is not acceptable as is.

You are overly optimistic when saying it is simple. It will involve
quite a bit of work: checking filters to see if their code is safe to
call several times; probably use some kind of flag to see which one are
safe; inserting scale if necessary; providing an API to enable/disable
parameters changes on buffersink; providing an API to detect parameters
changes conveniently.

Not overwhelmingly difficult, but not an easy task either.

And this is only speaking of changes in resolution and other shallow
parameters. If you want to allow changes in pixel format (which your
commits do), you need to adapt the whole format negotiation.

Regards,
Michael Niedermayer Dec. 3, 2017, 10:13 p.m.
On Sun, Dec 03, 2017 at 08:21:50PM +0100, Nicolas George wrote:
> Michael Niedermayer (2017-12-01):
> > Do you think this comment will fix any issue or improve anything ?
> 
> This comment had one goal: prevent somebody from pushing incorrect
> changes before I had time to make my point.
> 
> > First i do not even know what you speak of exactly and as this is
> 
> I posted the exact reference in my previous mail in this thread, which
> is not very long:
> 
> https://ffmpeg.org/pipermail/ffmpeg-devel/2017-December/221480.html
> 
> > supposedly about code i wrote, i doubt others will have a better idea.
> > 
> > And there is possibly a difference in goals. Reinitializing filters
> > would loose filter state, so avoiding reinit unless really needed was
> > a big goal. Last but not least what i had envissioned is just in my head,
> > i never implemeted it. (the bikeshedding made me loose
> > interrest back then as i already mentioned) what is in git is a small
> > part of it, that alone certainly is not that great.
> > and "too simple" yes my vission of this was and is simple
> 
> I agree that avoiding resets is a worthy goal. I do not know if what you
> had in your head was correct or not. But what you committed in
> 9225513242 (and related commits 5d2b885074, 6c702c3c63, 5d859e5980) was
> just wrong. And one of the reason we can be sure it was wrong is that it
> caused assert failures, that you did not fix but just hid with more of
> the same.

I think i may be missing something here

The way i remember this, is that all the changes do never get
triggered unless the user application passes frames with changing
parameters into the filter graph.
That is something not supported before these commits. So an application
first has to introduce changes to get changes out or to get any of
these failure modes. While before all this should have failed 


[...]
> 
> You are overly optimistic when saying it is simple. It will involve
> quite a bit of work: checking filters to see if their code is safe to
> call several times; probably use some kind of flag to see which one are
> safe; inserting scale if necessary; providing an API to enable/disable
> parameters changes on buffersink; providing an API to detect parameters
> changes conveniently.
> 
> Not overwhelmingly difficult, but not an easy task either.

I think our difference here is entirely linguistic

its a lot of work, and indeed as there are more filters now its
more work than what it would have been in the past.
But it is simple work (as in alot but it does not seem to be complicated work)


> 
> And this is only speaking of changes in resolution and other shallow
> parameters. If you want to allow changes in pixel format (which your
> commits do), you need to adapt the whole format negotiation.

iam not sure
naively the same approuch (flag & insert scale or reinit or do nothing)
should work too.
I do not think we need to rebuild parts of the filter graph totally
reoptimizing for changed pixel formats but we could of course if thats
something someone wants to implement


[...]
Nicolas George Dec. 10, 2017, 11:33 a.m.
Michael Niedermayer (2017-12-03):
> I think i may be missing something here
> 
> The way i remember this, is that all the changes do never get
> triggered unless the user application passes frames with changing
> parameters into the filter graph.
> That is something not supported before these commits. So an application
> first has to introduce changes to get changes out or to get any of
> these failure modes. While before all this should have failed 

Yes, you are right. The problem is not as severe as I made it. But it is
still a worry. These checks are there to protect from bugs. Or more
precisely to protect from the consequences of bugs, and these changes
weaken that protection.

And there are bugs. Our very own src_movie does not check for formats
changes in decoded frames. An application trusting it could write
outside the frame data, and cause security issues. It would be better if
it crashed on an assert failure, because assert failures, at the very
least, are not exploitable.

> iam not sure
> naively the same approuch (flag & insert scale or reinit or do nothing)
> should work too.
> I do not think we need to rebuild parts of the filter graph totally
> reoptimizing for changed pixel formats but we could of course if thats
> something someone wants to implement

That would be very suboptimal, but as long as somebody is willing to do
the work properly, why not.

Regards,

Patch hide | download patch | download mbox

diff --git a/doc/filters.texi b/doc/filters.texi
index 4a4efc70c8..e8da5e21a6 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -14057,6 +14057,10 @@  Set stereoscopic image format of input.
 
 Available values for input image formats are:
 @table @samp
+@item auto
+set input format using frame side data
+If such data is not available filter will return unchanged input.
+
 @item sbsl
 side by side parallel (left eye left, right eye right)
 
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index b98b32bacb..0d4bfd2671 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -378,6 +378,36 @@  int avfilter_config_links(AVFilterContext *filter)
     return 0;
 }
 
+static int reset_links(AVFilterContext *filter)
+{
+    int i, ret;
+
+    if (!filter)
+        return 0;
+
+    for (i = 0; i < filter->nb_outputs; i++) {
+        AVFilterLink *link = filter->outputs[i];
+
+        link->init_state = AVLINK_UNINIT;
+        link->frame_rate.num = link->frame_rate.den = 0;
+        link->w = link->h = 0;
+
+        ret = reset_links(link->dst);
+        if (ret < 0)
+            return ret;
+    }
+
+    if (!i)
+        return avfilter_config_links(filter);
+
+    return 0;
+}
+
+int ff_reconfig_links(AVFilterContext *filter)
+{
+    return reset_links(filter);
+}
+
 void ff_tlog_link(void *ctx, AVFilterLink *link, int end)
 {
     if (link->type == AVMEDIA_TYPE_VIDEO) {
diff --git a/libavfilter/internal.h b/libavfilter/internal.h
index f9679ed1d7..5062bb7296 100644
--- a/libavfilter/internal.h
+++ b/libavfilter/internal.h
@@ -411,4 +411,6 @@  static inline int ff_norm_qscale(int qscale, int type)
  */
 int ff_filter_get_nb_threads(AVFilterContext *ctx);
 
+int ff_reconfig_links(AVFilterContext *ctx);
+
 #endif /* AVFILTER_INTERNAL_H */
diff --git a/libavfilter/vf_stereo3d.c b/libavfilter/vf_stereo3d.c
index 8b22f880ca..6665a2631b 100644
--- a/libavfilter/vf_stereo3d.c
+++ b/libavfilter/vf_stereo3d.c
@@ -25,6 +25,7 @@ 
 #include "libavutil/opt.h"
 #include "libavutil/parseutils.h"
 #include "libavutil/pixdesc.h"
+#include "libavutil/stereo3d.h"
 #include "avfilter.h"
 #include "drawutils.h"
 #include "formats.h"
@@ -47,8 +48,8 @@  enum StereoCode {
     ANAGLYPH_YB_DUBOIS, // anaglyph yellow/blue dubois
     ANAGLYPH_RB_GRAY,   // anaglyph red/blue gray
     ANAGLYPH_RG_GRAY,   // anaglyph red/green gray
-    MONO_L,             // mono output for debugging (left eye only)
-    MONO_R,             // mono output for debugging (right eye only)
+    MONO_L,             // mono output (left eye only)
+    MONO_R,             // mono output (right eye only)
     INTERLEAVE_ROWS_LR, // row-interleave (left eye has top row)
     INTERLEAVE_ROWS_RL, // row-interleave (right eye has top row)
     SIDE_BY_SIDE_LR,    // side by side parallel (left eye left, right eye right)
@@ -66,7 +67,8 @@  enum StereoCode {
     INTERLEAVE_COLS_LR, // column-interleave (left eye first, right eye second)
     INTERLEAVE_COLS_RL, // column-interleave (right eye first, left eye second)
     HDMI,               // HDMI frame pack (left eye first, right eye second)
-    STEREO_CODE_COUNT   // TODO: needs autodetection
+    AUTO,               // guess input format using frame's metadata.
+    STEREO_CODE_COUNT
 };
 
 typedef struct StereoComponent {
@@ -173,6 +175,7 @@  static const AVOption stereo3d_options[] = {
     { "irr",   "interleave rows right first",         0, AV_OPT_TYPE_CONST, {.i64=INTERLEAVE_ROWS_RL}, 0, 0, FLAGS, "in" },
     { "icl",   "interleave columns left first",       0, AV_OPT_TYPE_CONST, {.i64=INTERLEAVE_COLS_LR}, 0, 0, FLAGS, "in" },
     { "icr",   "interleave columns right first",      0, AV_OPT_TYPE_CONST, {.i64=INTERLEAVE_COLS_RL}, 0, 0, FLAGS, "in" },
+    { "auto",  "guess using frame metadata",          0, AV_OPT_TYPE_CONST, {.i64=AUTO},               0, 0, FLAGS, "in" },
     { "out",   "set output format", OFFSET(out.format),  AV_OPT_TYPE_INT,   {.i64=ANAGLYPH_RC_DUBOIS}, 0, STEREO_CODE_COUNT-1, FLAGS, "out"},
     { "ab2l",  "above below half height left first",  0, AV_OPT_TYPE_CONST, {.i64=ABOVE_BELOW_2_LR},   0, 0, FLAGS, "out" },
     { "ab2r",  "above below half height right first", 0, AV_OPT_TYPE_CONST, {.i64=ABOVE_BELOW_2_RL},   0, 0, FLAGS, "out" },
@@ -366,6 +369,9 @@  static int config_output(AVFilterLink *outlink)
     int ret;
     s->aspect = inlink->sample_aspect_ratio;
 
+    if (s->in.format == AUTO || (s->in.format == s->out.format))
+        return 0;
+
     switch (s->in.format) {
     case INTERLEAVE_COLS_LR:
     case INTERLEAVE_COLS_RL:
@@ -670,6 +676,31 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *inpicref)
     int out_off_left[4], out_off_right[4];
     int i, ret;
 
+    if (s->in.format == AUTO) {
+        AVFrameSideData *sd = av_frame_get_side_data(inpicref, AV_FRAME_DATA_STEREO3D);
+        AVFilterLink *link;
+
+        if (sd) {
+            AVStereo3D *s3d = (AVStereo3D *)sd->data;
+
+            switch (s3d->type) {
+            case AV_STEREO3D_SIDEBYSIDE:          s->in.format = SIDE_BY_SIDE_LR;    break;
+            case AV_STEREO3D_SIDEBYSIDE_QUINCUNX: s->in.format = SIDE_BY_SIDE_LR;    break;
+            case AV_STEREO3D_TOPBOTTOM:           s->in.format = ABOVE_BELOW_LR;     break;
+            case AV_STEREO3D_CHECKERBOARD:        s->in.format = CHECKERBOARD_LR;    break;
+            case AV_STEREO3D_FRAMESEQUENCE:       s->in.format = ALTERNATING_LR;     break;
+            case AV_STEREO3D_LINES:               s->in.format = INTERLEAVE_ROWS_LR; break;
+            case AV_STEREO3D_COLUMNS:             s->in.format = INTERLEAVE_COLS_LR; break;
+            default:                              s->in.format = s->out.format = MONO_L;
+            };
+        } else {
+            s->in.format = s->out.format = MONO_L;
+        }
+
+        if ((ret = ff_reconfig_links(ctx)) < 0)
+            return ret;
+    }
+
     if (s->in.format == s->out.format)
         return ff_filter_frame(outlink, inpicref);