Created attachment 31387 [details] [review]
ximagesink PAR fixes.

retrieves pixel aspect ratio (PAR) from the xserver in _xcontext_get(). The
xcontext-caps is created in this function as well, including PAR. Due to user
configurability, a user can change the PAR and therefore, the final variable
containing the PAR is only intialized *after* xcontext_get() returns (actually,
it is being set right after calling xcontext_get() in change_state()). This
order is obviously wrong. You need to first set the PAR variable and then set
the caps. Attached patch fixes this. Similarly, the PAR is also free'ed in
xcontext_clear() rather than in _change_state(). No patch for xvimagesink is
provided because it handles PAR differently.

Created attachment 31388 [details] [review]
videoscale PAR fixes.

videoscale handles PAR solely in the _fixate() and _link() functions. It works
as follows: it waits for either pad to start negotiating, then it will try to
set a "fitting" caps on the other pad and set up scaling as that. There's a bug
in this logic, however. Firstly, the PAR and width/height variables of a
previous link attempt are being stored in the videoscale struct, and those are
being used for subsequent link attempts. This is wrong, because those values
might have changed. Secondly, some of those values are only being set *after*
negotiation of the other pad has succeeded, but are being used *to set up*
negotiation of this other pad. The order here is wrong.

What *should* happen is that, on a link attempt on some pad, we automagically
create a "fitting" caps for the other pad and try to set that. If it works, set
up and be happy, else moan and bail out. During each link attempt, we should
retrieve the current supported values for PAR and size from the otherpad before
trying to set it, and generate an optimal caps for the other pad from that in
the following order:
- passthru
- scale, but keeping PAR
- scale, changing PAR but keeping the PAR/width/height ratios intact
- scale and change PAR/width/height ratios
Only when all that is done, do we store the from/to_width/height in the
videoscale struct and use that for scaling in the _chain() function. The PAR
values can be discarded from here on because we only use them for linking: PAR
values are not needed for scaling itself.

One particular thing that grabbed my attention is that videoscale contains a
_fixate() function which calculates src size based on sink PAR (which must
already be there) and possible width/height/PAR values on the sink side. This
is wrong for two reasons:
- the link function can already do this, as my proposed patch does
- it's not bidirectional
Therefore, it should probably be removed. In my current patch, the code is
still there but is never actaully being used because either the sink PAR is
already set (and then we set a fixed caps on the src pad right in the _link()
function) or the sink PAR is not set yet.

Please comment.

Created attachment 31561 [details] [review]
updated videoscale patch

* fix software scaling in playbin
Due to a small bad in logic, it didn't actually take *peer* size instead of
*own* size into account; this broke software scaling in video players.

All committed. CVS has been broken long enough now.