Created attachment 307881 [details] [review]
update PAR calculations

as per discussion in IRC updating similar to avviddec.c

Review of attachment 307881 [details] [review]:

Looks OK at a first glance but:

::: gst/videoparsers/gsth263parse.c
@@ +238,3 @@
+  gst_h263_parse_get_par (params, &parser_par_num, &parser_par_denom);
+  GST_DEBUG_OBJECT (h263parse, "PAR from params, num %d, denom %d",
+        demuxer_par_num, demuxer_par_denom);

I think this message and the previous one seem
common enough to be _LOG instead

Created attachment 308422 [details] [review]
update PAR calculations

changed from debug to log as per review

FWIW, this only happens on negotiation and is not a recurrent message, so DEBUG would have been perfectly fine as wlel.

(In reply to Vineeth from comment #0)
> In case of h263 video format, libav is calculating the PAR as 12/11
> but qtdemux is overwriting it to 1/1, resulting in wrong PAR being passed on.
> So instead of just using the PAR from demuxer, check the PAR value in parser,
> and compare the demuxer and parser PAR to determine the actual PAR.
> 
> This will align the PAR calculation to how it is done in libav, gstavviddec.c

I would like to understand what you're trying to achieve here exactly.

Am I correct to think that it works correctly with avdec_h263* (because it does the same logic) ?

But you want to do this logic in h263parsers, so that other decoders get the right par from the start?

Or what is the goal here? (Not saying there's anything wrong with the patch, just wondering)

It started as a simple case of validate media check failing since media info had 12/11 and the caps are being set as 1/1..
and then on discussing this in IRC, thaytan suggested to make these changes :)

But who set caps with par=1/1? So you're saying that the H.263 libav decoder doesn't figure out the PAR from the h263 bitstream? Or does the media check not plug a decoder at all?

Tim, isn't the bug that validate discover 12/11 but decoder output it 1/1 due to the bug being fixed ?

This is what I'm trying to figure out :)

The decoder has the same logic (prefer non-1/1 PAR from bitstream if demuxer PAR is 1/1), so the decoder should output the correct PAR even without this patch, unless the libav decoder is not being used, or the libav decoder does not extract the non-1/1 PAR from the h263 bistream, or a different decoder is being used, or no decoder is being used by the discoverer. I'd like to know which case it is.

Ok I'm lost, Vineeth, do you have a stream so we can study/reproduce this issue ?

maybe my last message was confusing.. let me try to clarify here...

it can be reproduced with any h263 stream..
i used one from http://download.wavetlan.com/SVV/Media/HTTP/http-3gp.htm

so what happens here is 
in avdec_h263 pad, PAR is 12/11. This follows the logic (prefer non-1/1 PAR from bitstream if demuxer PAR is 1/1) as written in gstavviddec.c
in h263parse pad, PAR is 1/1. This happens because, with previous logic, h263parse checks its sink caps and if PAR is already present there, uses it by default.
Here qtdemux calculates it to 1/1 and sends it across to h263parse.

0:00:00.819054604 29351 0x7f1060008800 DEBUG               GST_CAPS gstpad.c:2574:gst_pad_get_current_caps:<h263parse0:src> get current pad caps video/x-h263, variant=(string)itu, width=(int)176, height=(int)144, framerate=(fraction)5000/6689, pixel-aspect-ratio=(fraction)1/1, parsed=(boolean)true, annex-d=(boolean)false, annex-e=(boolean)false, annex-f=(boolean)false, annex-g=(boolean)false, annex-i=(boolean)false, annex-j=(boolean)false, annex-k=(boolean)false, annex-m=(boolean)false, annex-n=(boolean)false, annex-q=(boolean)false, annex-r=(boolean)false, annex-s=(boolean)false, annex-t=(boolean)false, annex-u=(boolean)false, annex-v=(boolean)false, profile=(string)0, level=(string)10
0:00:00.819139260 29351 0x7f1060008800 DEBUG               GST_CAPS gstpad.c:2574:gst_pad_get_current_caps:<qtdemux0:video_0> get current pad caps video/x-h263, variant=(string)itu, width=(int)176, height=(int)144, framerate=(fraction)5000/6689, pixel-aspect-ratio=(fraction)1/1

0:00:00.843486064 29351 0x7f1060008800 DEBUG               GST_CAPS gstpad.c:2574:gst_pad_get_current_caps:<avdec_h263-0:src> get current pad caps video/x-raw, format=(string)I420, width=(int)176, height=(int)144, interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)12/11, chroma-site=(string)jpeg, colorimetry=(string)bt601, framerate=(fraction)5000/6689

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/275.