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

[Ivan Kalvachev]

Tobias Diedrich said:
> 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).
You can't count. They are 3 ;)
We don't skip aspect reading for other containers that have
aspect info. There is no (yet) reason to do this for avi.
There is no reason not to write aspect info in avi, if
we know the aspect.
Are there options to disable OpenDML support?

>
>> 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.
OK, I was wrong.

>
>> "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.
OK, then the only option will be "forceaviaspect"

>> 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.
Some ve_encoders do calculate aspect. I will update the rest.
And it is more logical encoder to calculate aspect
and pass it to it's muxer, rather than muxer jump
over encoder, land in video filters and collect info
on it's own.
I'm sure that you are aware that the d_width/d_height you want
are passed to the encoder. No need to hack the video system, to
get it.

I'm afraid that you took dalias words literally.
He mostly mean that you should have take the aspect
from the end of video system, not that the only
way to do it is to read if directly from vf.

> As such it would already work with other formats than avi.
Your hack should be moved for every future muxer.
Every muxer should parse the video system by its own.
It should calculate it's own aspect, and it may be different
than the one calculated by ve_encoder.

> 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.
DEMUXER SHOULD NOT MESS WITH VIDEO FILTERS!
Unless there is a very,very good reason.
What don't you understand with this?

>
>> 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.
There are other ways this to be done, as I have showed.
And the other way is 1 line (well 2 lines)
Why the muxer should take the aspect information DIRECTLY
from video system?
You are implementing your own filter parser.
This is a mess. You need only the end result.
And as I said the end is video encoder. Video lavc encoder
calculate aspect ANYWAY, so you just have to store it.

>
>> In the way you make it you can easly break not only aspect.
>> d_width/d_height could have valid value of  (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.
Read the DOCS/tech/.
They are only hints! All vo_outputs have explict check for 0.
It is a legacy that may have been nearly fixed, but...


>
>> 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).
OMG. This should be done only by the video system core. Not by some rundom
muxer!

>
>> 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.
Do you know what you are doing?
Why just don't use that -lavcopts aspect=x/y as avi_aspect if the
user have not forced another forceaviaspect?

>
>> 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"
Sorry, I read the wrong version of avi_aspect(), the one that read float


If I had an working version of mplayer, i've could have
finish my patch.
And I still have to investigate why i get 0x0 size avi.
(sorry forgot the file at home):

Best Regards
   Ivan Kalvachev
  iive

p.s.
Sorry if i'm a little jumpy. But take a look of the possible
future uses. When we implement g2 vf and we start porting
g1 modules, we may have a tree-like video filter system, that
cannot be walked by your muxer, and it may have multiple
encoders/outputs that produce multiple streams and outputs
at once.