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 685103 - mpegvideoparse: wrong pixel-aspect-ratio
mpegvideoparse: wrong pixel-aspect-ratio
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other Linux
: Normal normal
: 1.0.8
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-09-29 17:01 UTC by Tim-Philipp Müller
Modified: 2013-07-05 23:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpeg2dec: make the aspect ratio calculation more accurate. (1.42 KB, patch)
2013-02-19 10:25 UTC, sreerenj
none Details | Review
configure: bump required libmpeg2 to 0.5.0 (july 12,2008) (808 bytes, patch)
2013-02-19 10:26 UTC, sreerenj
none Details | Review
mpeg2dec: make the aspect ratio calculation more accurate. (1.46 KB, patch)
2013-06-04 13:16 UTC, sreerenj
committed Details | Review
mpegvideoparser: Fix the pixel-aspect-ratio calculation. (1.78 KB, patch)
2013-06-05 13:19 UTC, sreerenj
committed Details | Review

Description Tim-Philipp Müller 2012-09-29 17:01:50 UTC
With this video:

https://bugs.launchpad.net/ubuntu/+source/totem/+bug/705361/+attachment/1816551/+files/vlc-record-2011-01-29-22h40m45s-VTS_01_2.VOB-.mpg.zip

mpegvideoparse reports a PAR of 24/11 in 0.10, and a PAR of 16/15 with 1.0, which looks wrong.
Comment 1 sreerenj 2012-11-08 08:30:42 UTC
This video has display_extension which is giving a display_horizatal_size of 720 and display_vertical_size of 576. The PAR is calculating in 1.0 based on this instead of the default width of 352 and height of 576 .
(default widhth/height : valued parsed/calculated from SequenceHdr and SequenceHdrExtension)

But in 0.10 there is no parsing for display_extension and which will use the default width of 352 instead of 720 for PAR calculation.

It seems that the mpeg2dec is giving the PAR 16/15 in both 0.10 and 1.0.(pipeline with out parser element) . But avdec_mpeg2video/ffdec_mpeg2video is giving 24/11 (with out parser element).
Comment 2 sreerenj 2013-01-29 12:54:41 UTC
If i understood correctly, this is not a bug in 1.0 but instead a "missing feature" in 0.10.
Comment 3 Tim-Philipp Müller 2013-01-29 13:26:12 UTC
> If i understood correctly, this is not a bug in 1.0 but instead a "missing
> feature" in 0.10.

Sorry, could you explain that remark? Are you saying it is displayed correctly in 1.0?
Comment 4 sreerenj 2013-01-29 13:34:01 UTC
I think i explained it in comment 1. This video has a display_extension which is not parsing by the 0.10 parser element. Because there was no API for that.
Right now the 1.0 parser is parsing seq_display_ext and calculating correct PAR using gst_mpeg_video_finalise_mpeg2_sequence_header().

Am I missing something ? :)
Comment 5 Tim-Philipp Müller 2013-01-29 14:11:36 UTC
Let me ask differently:

a) does this play with correct dimensions for you using
    .. ! mpegvideoparse ! {mpeg2dec | avdec_mpeg2video } ! xvimagesink ?
   (it does not for me, the video is about twice as high as it is wide;
    it plays fine with mpegpsdemux ! avdec_mpeg2video ! xvimagesink)

b) do you think it is correct that it is twice as high as it is wide?
Comment 6 sreerenj 2013-01-29 14:44:01 UTC
In 0.10
---------------
case1: with mpegvideoparse, 

gst-launch -v filesrc location=/home/sreerenj/samples/mpeg2/vlc-record-2011-01-29-22h40m45s-VTS_01_2.VOB-.mpg  ! mpegpsdemux ! mpegvideoparse ! queue ! mpeg2dec ! xvimagesink 

will give the PAR of 24/11.  because of wrong value from mpegvideoparse.(as i explained in comment 1)


case2: with out mpegvideoparse,

gst-launch -v filesrc location=/home/sreerenj/samples/mpeg2/vlc-record-2011-01-29-22h40m45s-VTS_01_2.VOB-.mpg  ! mpegpsdemux !  queue ! mpeg2dec ! xvimagesink

will give par of 16/15 and video seems to be playing as half. I think mpeg2dec is correctly parsing the disply_extension here. And the same result is getting in 1.0 with mpegvideoparse.

What i am saying it that , the PAR calculation seems right in 1.0.
Comment 7 sreerenj 2013-01-29 15:23:38 UTC
ASFAIK, the display_horizontal_size/display_verical_size for disply_extension is only using for PAR calculation  at the moment.

We might also need to pass it to the downstream element as a video_cropping parameter....
Comment 8 Tim-Philipp Müller 2013-01-29 15:40:33 UTC
I don't really care too much about what we did in 0.10, but vlc and mplayer play this clip with a 4:3 aspect ratio, and it looks like that's how it's supposed to be played. Surely that's not just coincidence? I want GStreamer to play the clip with a 4:3 aspect ratio as well. So it seems to me like either there's something we're missing, or we're doing the right thing but the data in the extension is bogus, and we should figure out somehow that it's bogus. The video *decoders* should ignore any aspect ratio information they find in the bitstream if upstream (demuxer and/or parser) provide one, so I believe it should be mpegvideoparse's job in this case to figure out the right thing.
Comment 9 sreerenj 2013-01-29 15:52:38 UTC
Yup, vlc is playing with almost double size  :)..Is it the case which needs frame-cropping support? Do we support frame cropping?,,,which means, if the encoded data specify the cropping region? 

As per the spec , "display_horizontal_size and display_vertical_size together define a rectangle which may be considered as the "intended display's" active region. If this rectangle is smaller than the encoded frame size, then the
display process may be expected to display only a portion of the encoded frame. Conversely if the display rectangle is larger than the encoded frame size, then the display process may be expected to display the reconstructed frames on a
portion of the display device rather than on the whole display device. "

"display_horizontal_size and display_vertical_size do not affect the decoding process but may be used by the displayprocess that is not standardised in this Specification.
"
We are not passing the display_horizontal_size to downstream elements. Which means,

here width=352, height=576, display_horizontal_size=720.
PAR is calculating based on "720" . But video-sink always using the default width "352".
Comment 10 sreerenj 2013-02-11 12:52:18 UTC
Adding VideoCropMeta to mpeg2dec with display_ext->width and display_ext->height may be a fix for this.(don't know whether it is going to fix the issue...).Or is there some way to communicate the display region to the renderer??
Comment 11 David Schleef 2013-02-11 21:39:54 UTC
We should just do what DVD players do, as this is outside the MPEG-2 spec.  I'm guessing that when display_horizontal_size > width, this means we should stretch the video to match the display size.  In other words, ignore the display_horizontal_size and calculate the PAR based on the video size.

For display_horizontal_size < width, I am not sure.  It would be best to check pan-scan content to make sure it works.
Comment 12 sreerenj 2013-02-19 10:25:20 UTC
Created attachment 236734 [details] [review]
 mpeg2dec: make the aspect ratio calculation more  accurate.

This is not a fix for the current issue. Just an enhancement to mpeg2dec.
Comment 13 sreerenj 2013-02-19 10:26:00 UTC
Created attachment 236735 [details] [review]
configure: bump required libmpeg2 to 0.5.0 (july  12,2008)
Comment 14 sreerenj 2013-06-04 13:16:21 UTC
Created attachment 246003 [details] [review]
mpeg2dec: make the aspect ratio calculation more accurate.

Merged the previous two patches with slight changes. Instead of bumping the libmpeg2 version , use version check in gstmpeg2dec.c.
Comment 15 Sebastian Dröge (slomo) 2013-06-04 15:31:18 UTC
commit 866f60cce6ec61407b3b2f5e9dc42610da9bba61
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Tue Jun 4 16:12:27 2013 +0300

    mpeg2dec: make the aspect ratio calculation more accurate.
    
    Utilize the libmpeg2 api: mpeg2_guess_aspect() to guess the
    aspect-ratio if we don't have a valid upstream PAR.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=685103
Comment 16 sreerenj 2013-06-04 18:55:22 UTC
The patch is not a fix the issue mentioned in this bug but just an enhancement to the mpeg2dec.
Comment 17 Sebastian Dröge (slomo) 2013-06-05 11:03:05 UTC
I think what David said here makes sense and is easy enough to implement
Comment 18 sreerenj 2013-06-05 13:19:11 UTC
Created attachment 246074 [details] [review]
mpegvideoparser: Fix the pixel-aspect-ratio calculation.
Comment 19 Sebastian Dröge (slomo) 2013-06-06 12:33:13 UTC
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Wed Jun 5 16:16:36 2013 +0300

    mpegvideoparser: Fix the pixel-aspect-ratio calculation
    
    Ignore the display_extension values if they are greater than the width/heigh
    values provided by seqhdr and calculate the PAR based on the seqhdr values.T
    his is what DVD players are doing.
    
    Thanks to "David Schleef <ds@schleef.org>"
    
    https://bugzilla.gnome.org/show_bug.cgi?id=685103
Comment 20 Tim-Philipp Müller 2013-07-05 23:47:43 UTC
Also picked into 1.0 branch.