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 736396 - isomp4: duplicate if else branches in atoms.c
isomp4: duplicate if else branches in atoms.c
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.5.1
Assigned To: Reynaldo H. Verdejo Pinochet
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-10 09:50 UTC by Anuj
Modified: 2018-10-22 11:23 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
isomp4: duplicate if else branches in atoms.c (928 bytes, patch)
2014-09-10 09:50 UTC, Anuj 		none Details | Review
isomp4: duplicate if else condition (revised) (970 bytes, patch)
2014-09-11 08:33 UTC, Felix Schwarz 		rejected Details | Review
Fix wrong DAR computation for PAR <= 1 (866 bytes, patch)
2014-09-11 16:53 UTC, Reynaldo H. Verdejo Pinochet 		none Details | Review
Fix wrong DAR computation for PAR <= 1 (1.74 KB, patch)
2014-09-16 21:39 UTC, Reynaldo H. Verdejo Pinochet 		committed Details | Review

Description Anuj 2014-09-10 09:50:52 UTC 
Created attachment 285809 [details] [review]
isomp4: duplicate if else branches in atoms.c 

In gst-plugins-good:
isomp4/atom.c found duplicate branches for if else, removed it and added a patch for same, please review.
Comment 1 Felix Schwarz 2014-09-11 08:32:14 UTC 
your patch looks strange as you only remove two empty lines.
Comment 2 Felix Schwarz 2014-09-11 08:33:06 UTC 
Created attachment 285888 [details] [review]
isomp4: duplicate if else condition (revised)

I think you meant to change the code like this, right?
Comment 3 Anuj 2014-09-11 08:38:25 UTC 
yes actually i removes those in first place but i thought there was some error in git so its like that i'll resubmit the patch. Thanks for your review
Comment 4 Reynaldo H. Verdejo Pinochet 2014-09-11 16:48:07 UTC 
Review of attachment 285888 [details] [review]:

The problem is likely not the code been repeated
but the calculation being wrong on the else branch
Comment 5 Reynaldo H. Verdejo Pinochet 2014-09-11 16:53:53 UTC 
Created attachment 285936 [details] [review]
Fix wrong DAR computation for PAR <= 1

Of the top of my head, not sure it's correct. Please review
Comment 6 Reynaldo H. Verdejo Pinochet 2014-09-11 17:00:14 UTC 
Additionally, Using gst_video_calculate_display_ratio()
might be the best option anyway.
Comment 7 Reynaldo H. Verdejo Pinochet 2014-09-16 21:39:55 UTC 
Created attachment 286326 [details] [review]
Fix wrong DAR computation for PAR <= 1

Alternative using _calculate_display_ratio()
Comment 8 Thiago Sousa Santos 2014-09-17 02:08:07 UTC 
Review of attachment 286326 [details] [review]:

Looks good.
Comment 9 Reynaldo H. Verdejo Pinochet 2014-09-18 21:55:06 UTC 
commit e655d47dfce1652630fe8ff5fb6be56370087004
Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com>
Date:   Thu Sep 11 13:48:44 2014 -0300

    isomp4: fix wrong DAR calculation for PAR <= 1
    
    CID #1226452
    
    https://bugzilla.gnome.org/show_bug.cgi?id=736396
Comment 10 Sebastian Dröge (slomo) 2018-10-22 11:19:47 UTC 
This is wrong: it puts a width/height of the display aspect ratio in there, e.g. width 4 and height 3.

The original code was correct, it was just duplicating the calculation for both cases for whatever reason but the calculating was correct for both cases.