After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 555699 - [PATCH] theoradec: prefer container's pixel aspect ratio over the stream's
[PATCH] theoradec: prefer container's pixel aspect ratio over the stream's
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-10-09 16:29 UTC by Robin Stocker
Modified: 2008-10-14 21:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
prefer container's PAR over stream's in theoradec.c (1.38 KB, patch)
2008-10-09 16:33 UTC, Robin Stocker
reviewed Details | Review

Description Robin Stocker 2008-10-09 16:29:25 UTC
When a Theora video stream is inside the Matroska container and a pixel-aspect-ratio is specified in the container, it is not respected. Instead, theoradec overwrites the PAR with the values the stream itself provides. But the PAR of the container should take precedence over what the stream says because a user usually sets it with the container.

The attached patch (against HEAD) uses the PAR provided by the container if there is one and falls back to the one from the stream itself if not. Note that the patch is kind of a proof of concept, the comment around that code should also be changed and I'm not sure if the unref is right.

I noticed that behaviour because Totem with the GStreamer backend seemed to ignore Matroska's display-dimensions. And it's not just with Theora streams, MPEG streams for example also have that problem. Is changing the decoders like in the patch the right way to solve this? If yes, I can look at the other decoders and try to come up with patches.
Comment 1 Robin Stocker 2008-10-09 16:33:39 UTC
Created attachment 120284 [details] [review]
prefer container's PAR over stream's in theoradec.c
Comment 2 Michael Smith 2008-10-09 17:10:10 UTC
Obviously, there's no way to be completely correct here - we'll either be wrong with files that have the right PAR in the codec data, or we'll be wrong with files that have the right PAR in the container. 

I _think_ it's done the current way because our experience was that files with the correct PAR in the codec data are much more common - so we don't want to use the container's PAR if the codec provides one. I think there are already some exceptions to this in some of the decoders (e.g. if the container specifies a non-square PAR, and the codec has a square PAR, then use the container PAR).

So in general I don't think we want to do this, but there may be some reasonable heuristics you could come up with to fix some files?


Comment 3 Robin Stocker 2008-10-10 10:08:12 UTC
Thanks for the quick and detailed response Michael!

Yes, that sounds like a good idea. Where the user didn't take care to set the correct DAR for the container it would most likely be square or the same as the stream itself (mkvmerge does it like that) anyway.

So, I looked at some files and found that my problems are mostly with MPEG videos (from DVDs in 720x576). The stream itself has an incorrect 16:15 PAR (4:3 DAR) but they are muxed in an MKV with the correct 64:45 PAR (16:9 DAR). So if neither PAR is square, what should we do?

One solution is to take the PAR of the container. Another is to take the PAR that is less square (64:45). Both solutions work in this case, but I like the first better, because it's less arbitrary. I know there are many MPEG streams with an incorrect PAR, but I don't think that there are many files where the container specifies a different non-square PAR that is wrong. An argument for the second solution is that there are more videos on YouTube where the heads are too high than where the heads are too wide ;). What do you think?
Comment 4 Wim Taymans 2008-10-10 10:41:24 UTC
We have to do what everything else is doing because the tools would target the common practice. In this case, we should let the container aspect ratio override the encoded ratio. If there is no container aspect ratio, we don't set one in the caps and the decoder can use the encoded ratios.

DVDs are another case because there the aspect ratio is encoded into the IFO files and must be conveyed to the decoder through other means (like the aspect ration fixer element in resindvd)
Comment 5 Robin Stocker 2008-10-10 22:00:43 UTC
So that is pretty much what my initial patch for theoradec does. Can someone review it?

I also have a patch for mpeg2dec, should I open another bug report or attach it here?
Comment 6 Wim Taymans 2008-10-13 11:36:20 UTC
        Based on patch by: Robin Stocker <robin at nibor dot org>

        * ext/theora/gsttheoradec.h:
        * ext/theora/theoradec.c: (gst_theora_dec_init),
        (theora_dec_setcaps), (theora_handle_type_packet),
        (theora_dec_decode_buffer), (theora_dec_change_state):
        Parse input caps and make the PAR override the encoded PAR when
        specified by a container. Fixes #555699.
Comment 7 Robin Stocker 2008-10-13 20:12:26 UTC
For future reference, a patch (based on Wim Taymans's patch) for the same problem with mpeg2dec is in bug 556184.
Comment 8 Robin Stocker 2008-10-14 21:26:12 UTC
And another one for ffmpeg in bug 556336.