[MPlayer-dev-eng] Re: OpenDML Read/Write support

[Tobias Diedrich]

Ivan Kalvachev wrote:

> OMG,
> What a mess!
> I was too humble, saying my patch is not good enough.
> BTW Your patch is rejected.
> 
> 1.
> Sorry but I'm really getting confused.
> You had added a numerous options about avi aspect.

Numerous?
More like two.
-useaviaspect/-nouseaviaspect can be used to enabled/disable
the code reading and write the vprp header. (Well -nouseaviaspect is a
bit awkward I think, maybe it should be renamed)
-forceaviaspect is there to either override the aspect derived from the
end of the video filter chain or to set it if it can't be derived
because no video filter chain is used (-ovc copy).

> AFAIK mplayer override container aspect with aspect from the stream.
> This mean that avi aspect will be used (by mplayer) only for formats
> that do not have their own aspects (MPEG have)

This is not the case.  If you look at vd_ffmpeg, it only sets aspect if
it has not been set already.

> "forceaviaspect" should be "aviaspect" with default value -1.0

IMHO it should _not_ be called just "aviaspect" as that would encourage
users to use it to set it and not only to force/override the aspect
setting.

> If it is not set the value that encoder calculate will be
> encoded into the avi. If encoder doesn't calculate
> aspect then avi won't have aspect info.

The encoder doesn't have to calculate aspect, it gets done by the video
filter chain and the final filter (encoder) is called with the aspect
encoded in d_width and d_height.
As such it would already work with other formats than avi.
The corresponding part of the patch only changes five lines of the vf
code, so that the aspect information is retained and no changes to the
various ve_*.c are necessary.

> I accept the mencoder change, probably it could be
> reversed in the cvs.

If it still is the last commit to mencoder.c, then yes, it could be
reversed.

> You have messed really badly the video filter system
> And you have done it for nothing.

???
I only added 5 lines, that don't change existing behaviour at all.
How is that "badly messing"?
And it is not for nothing, but so that the muxer can get this aspect
information.

> In the way you make it you can easly break not only aspect.
> d_width/d_height could have valid value of 0 (zero) that
> meen original width/heigh is used.

AFAICS d_width/d_height is always != 0.
In a few parts d_width is unconditionally divided by d_height 
to get the aspect, without checking for d_height != 0.

> More over, if the way you had implemented it, you would get
> always the d_widht/d_height of the first filter!!!

No.
The value the filters config function was called with is retained for
all filters in the vf_instance_t struct and the avimuxer walks the
filter chain and uses the value from the last filter (the encoder).

> Even if you get the one from the last filter this would be
> bogus because as I said the last one is video encoder (ve_)!

Why would that mean it is bogus?
The encoder doesn't decide the aspect by itself, it either uses the one
provided by the filter chain (d_width/d_height) (-lavcopts autoaspect) or
the user provided override  (-lavcopts aspect=x/y).
For the latter one I added a warning that overriding is discouraged and
should only be used if the user knows what he is doing.

> lavc aspect exsist for a reason. It is not guarantieed that
> all video filters could hanlde aspect correctly.

Then those filters should be fixed.

> The aspect from container (avi) should be the same as the one
> in the stream. How your patch garantee that!

IMHO Going with unix philosophy we should only discourage the
user from shooting himself into the foot, but not prevent it.

> What the hell is muxer->def_v->source? And why you store
> one aspect info there and not another??????????!

Look at muxer.h, it clearly says
"void* source; // sh_audio or sh_video"

-- 
Tobias						PGP: http://9ac7e0bc.2ya.com
Be vigilant!