Fix integer overflow problem with pixel-aspect-ratio calculations in videoscale and xvimagesink (#341542)

Original commit message from CVS:
* docs/libs/gst-plugins-base-libs-docs.sgml:
* docs/libs/gst-plugins-base-libs-sections.txt:
* gst-libs/gst/video/video.c: (gst_video_calculate_display_ratio):
* gst-libs/gst/video/video.h:
* gst/videoscale/Makefile.am:
* gst/videoscale/gstvideoscale.c: (gst_video_scale_fixate_caps):
* sys/xvimage/xvimagesink.c: (gst_xvimagesink_setcaps):
* tests/check/Makefile.am:
* tests/check/libs/video.c: (GST_START_TEST), (video_suite),
(main):
Fix integer overflow problem with pixel-aspect-ratio calculations
in videoscale and xvimagesink (#341542)

10 files changed, 232 insertions, 18 deletions

diff --git a/ChangeLog b/ChangeLog
index 1fc1c1f0..113b506f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2006-05-12  Jan Schmidt  <thaytan@mad.scientist.com>
+
+	* docs/libs/gst-plugins-base-libs-docs.sgml:
+	* docs/libs/gst-plugins-base-libs-sections.txt:
+	* gst-libs/gst/video/video.c: (gst_video_calculate_display_ratio):
+	* gst-libs/gst/video/video.h:
+	* gst/videoscale/Makefile.am:
+	* gst/videoscale/gstvideoscale.c: (gst_video_scale_fixate_caps):
+	* sys/xvimage/xvimagesink.c: (gst_xvimagesink_setcaps):
+	* tests/check/Makefile.am:
+	* tests/check/libs/video.c: (GST_START_TEST), (video_suite),
+	(main):
+	  Fix integer overflow problem with pixel-aspect-ratio calculations
+	  in videoscale and xvimagesink (#341542)
+
 2006-05-12  Tim-Philipp Müller  <tim at centricular dot net>
 
 	* gst-libs/gst/tag/gstid3tag.c:
diff --git a/docs/libs/gst-plugins-base-libs-docs.sgml b/docs/libs/gst-plugins-base-libs-docs.sgml
index 2ea539b2..a31dab47 100644
--- a/docs/libs/gst-plugins-base-libs-docs.sgml
+++ b/docs/libs/gst-plugins-base-libs-docs.sgml
@@ -10,6 +10,7 @@
 <!ENTITY GstAudioSink SYSTEM "xml/gstaudiosink.xml">
 <!ENTITY GstBaseAudioSink SYSTEM "xml/gstbaseaudiosink.xml">
 <!ENTITY GstCddaBaseSrc SYSTEM "xml/gstcddabasesrc.xml">
+<!ENTITY GstVideo SYSTEM "xml/gstvideo.xml">
 <!ENTITY GstVideoSink SYSTEM "xml/gstvideosink.xml">
 <!ENTITY GstVideoFilter SYSTEM "xml/gstvideofilter.xml">
 <!ENTITY GstColorBalance SYSTEM "xml/gstcolorbalance.xml">
@@ -57,6 +58,7 @@ This library should be linked to by getting cflags and libs from
 <filename>gstreamer-plugins-base.pc</filename> and adding
 <filename>-lgstvideo-&GST_MAJORMINOR;</filename> to the library flags.
     </para>
+    &GstVideo;
     &GstVideoSink;
     &GstVideoFilter;
   </chapter>
diff --git a/docs/libs/gst-plugins-base-libs-sections.txt b/docs/libs/gst-plugins-base-libs-sections.txt
index 672162a2..84ccfa91 100644
--- a/docs/libs/gst-plugins-base-libs-sections.txt
+++ b/docs/libs/gst-plugins-base-libs-sections.txt
@@ -207,6 +207,11 @@ GstVideoFilter
 GstVideoFilterClass
 </SECTION>
 
+<SECTION>
+<FILE>gstvideo</FILE>
+<INCLUDE>gst/video/video.h</INCLUDE>
+gst_video_calculate_display_ratio
+</SECTION>
 
 <SECTION>
 <FILE>private</FILE>
diff --git a/gst-libs/gst/video/video.c b/gst-libs/gst/video/video.c
index 52a969ec..1273efd9 100644
--- a/gst-libs/gst/video/video.c
+++ b/gst-libs/gst/video/video.c
@@ -24,6 +24,18 @@
 
 #include "video.h"
 
+/**
+ * SECTION:gstvideo
+ * @short_description: Support library for video operations
+ *
+ * <refsect2>
+ * <para>
+ * This library contains some helper functions and includes the 
+ * videosink and videofilter base classes.
+ * </para>
+ * </refsect2>
+ */
+
 /* This is simply a convenience function, nothing more or less */
 const GValue *
 gst_video_frame_rate (GstPad * pad)
@@ -97,3 +109,77 @@ gst_video_get_size (GstPad * pad, gint * width, gint * height)
 
   return TRUE;
 }
+
+/**
+ * gst_video_calculate_display_ratio:
+ * @dar_n: Numerator of the calculated display_ratio
+ * @dar_d: Denominator of the calculated display_ratio
+ * @video_width: Width of the video frame in pixels
+ * @video_height: Height of the video frame in pixels
+ * @video_par_n: Numerator of the pixel aspect ratio of the input video.
+ * @video_par_d: Denominator of the pixel aspect ratio of the input video.
+ * @display_par_n: Numerator of the pixel aspect ratio of the display device
+ * @display_par_d: Denominator of the pixel aspect ratio of the display device
+ *
+ * Given the Pixel Aspect Ratio and size of an input video frame, and the 
+ * pixel aspect ratio of the intended display device, calculates the actual 
+ * display ratio the video will be rendered with.
+ *
+ * Returns: A boolean indicating success and a calculated Display Ratio in the 
+ * dar_n and dar_d parameters. 
+ * The return value is FALSE in the case of integer overflow or other error. 
+ *
+ * Since: 0.10.7
+ */
+gboolean
+gst_video_calculate_display_ratio (guint * dar_n, guint * dar_d,
+    guint video_width, guint video_height,
+    guint video_par_n, guint video_par_d,
+    guint display_par_n, guint display_par_d)
+{
+  gint num, den;
+
+  GValue display_ratio = { 0, };
+  GValue tmp = { 0, };
+  GValue tmp2 = { 0, };
+
+  g_return_val_if_fail (dar_n != NULL, FALSE);
+  g_return_val_if_fail (dar_d != NULL, FALSE);
+
+  g_value_init (&display_ratio, GST_TYPE_FRACTION);
+  g_value_init (&tmp, GST_TYPE_FRACTION);
+  g_value_init (&tmp2, GST_TYPE_FRACTION);
+
+  /* Calculate (video_width * video_par_n * display_par_d) /
+   * (video_height * video_par_d * display_par_n) */
+  gst_value_set_fraction (&display_ratio, video_width, video_height);
+  gst_value_set_fraction (&tmp, video_par_n, video_par_d);
+
+  if (!gst_value_fraction_multiply (&tmp2, &display_ratio, &tmp))
+    goto error_overflow;
+
+  gst_value_set_fraction (&tmp, display_par_d, display_par_n);
+
+  if (!gst_value_fraction_multiply (&display_ratio, &tmp2, &tmp))
+    goto error_overflow;
+
+  num = gst_value_get_fraction_numerator (&display_ratio);
+  den = gst_value_get_fraction_denominator (&display_ratio);
+
+  g_value_unset (&display_ratio);
+  g_value_unset (&tmp);
+  g_value_unset (&tmp2);
+
+  g_return_val_if_fail (num > 0, FALSE);
+  g_return_val_if_fail (den > 0, FALSE);
+
+  *dar_n = num;
+  *dar_d = den;
+
+  return TRUE;
+error_overflow:
+  g_value_unset (&display_ratio);
+  g_value_unset (&tmp);
+  g_value_unset (&tmp2);
+  return FALSE;
+}
diff --git a/gst-libs/gst/video/video.h b/gst-libs/gst/video/video.h
index bff86d58..a839da6a 100644
--- a/gst-libs/gst/video/video.h
+++ b/gst-libs/gst/video/video.h
@@ -189,6 +189,11 @@ gboolean gst_video_get_size   (GstPad *pad,
                                gint   *width,
                                gint   *height);
 
+gboolean gst_video_calculate_display_ratio (guint *dar_n, guint *dar_d,
+            guint video_width, guint video_height, 
+            guint video_par_n, guint video_par_d, 
+            guint display_par_n, guint display_par_d);
+
 G_END_DECLS
 
 #endif /* __GST_VIDEO_H__ */
diff --git a/gst/videoscale/Makefile.am b/gst/videoscale/Makefile.am
index 1ef36889..61727708 100644
--- a/gst/videoscale/Makefile.am
+++ b/gst/videoscale/Makefile.am
@@ -7,7 +7,9 @@ libgstvideoscale_la_SOURCES = \
 
 libgstvideoscale_la_CFLAGS = $(GST_CFLAGS) $(GST_BASE_CFLAGS) $(LIBOIL_CFLAGS)
 libgstvideoscale_la_LDFLAGS = $(GST_PLUGIN_LDFLAGS)
-libgstvideoscale_la_LIBADD = $(GST_BASE_LIBS) $(GST_LIBS) $(LIBOIL_LIBS)
+libgstvideoscale_la_LIBADD = \
+	$(top_builddir)/gst-libs/gst/video/libgstvideo-$(GST_MAJORMINOR).la \
+	$(GST_BASE_LIBS) $(GST_LIBS) $(LIBOIL_LIBS)
 
 noinst_HEADERS = \
 	gstvideoscale.h \
diff --git a/gst/videoscale/gstvideoscale.c b/gst/videoscale/gstvideoscale.c
index fe32eb64..c5fc666d 100644
--- a/gst/videoscale/gstvideoscale.c
+++ b/gst/videoscale/gstvideoscale.c
@@ -544,7 +544,6 @@ gst_video_scale_fixate_caps (GstBaseTransform * base, GstPadDirection direction,
 
   /* we have both PAR but they might not be fixated */
   if (from_par && to_par) {
-    GValue to_ratio = { 0, };   /* w/h of output video */
     gint from_w, from_h, from_par_n, from_par_d, to_par_n, to_par_d;
     gint count = 0, w = 0, h = 0, num, den;
 
@@ -580,11 +579,13 @@ gst_video_scale_fixate_caps (GstBaseTransform * base, GstPadDirection direction,
     gst_structure_get_int (ins, "width", &from_w);
     gst_structure_get_int (ins, "height", &from_h);
 
-    g_value_init (&to_ratio, GST_TYPE_FRACTION);
-    gst_value_set_fraction (&to_ratio, from_w * from_par_n * to_par_d,
-        from_h * from_par_d * to_par_n);
-    num = gst_value_get_fraction_numerator (&to_ratio);
-    den = gst_value_get_fraction_denominator (&to_ratio);
+    if (!gst_video_calculate_display_ratio (&num, &den, from_w, from_h,
+            from_par_n, from_par_d, to_par_n, to_par_d)) {
+      GST_ELEMENT_ERROR (base, CORE, NEGOTIATION, (NULL),
+          ("Error calculating the output scaled size - integer overflow"));
+      return;
+    }
+
     GST_DEBUG_OBJECT (base,
         "scaling input with %dx%d and PAR %d/%d to output PAR %d/%d",
         from_w, from_h, from_par_n, from_par_d, to_par_n, to_par_d);
diff --git a/sys/xvimage/xvimagesink.c b/sys/xvimage/xvimagesink.c
index b93fb6dc..78af43a5 100644
--- a/sys/xvimage/xvimagesink.c
+++ b/sys/xvimage/xvimagesink.c
@@ -125,6 +125,8 @@
 #include <gst/interfaces/navigation.h>
 #include <gst/interfaces/xoverlay.h>
 #include <gst/interfaces/colorbalance.h>
+/* Helper functions */
+#include <gst/video/video.h>
 
 /* Object header */
 #include "xvimagesink.h"
@@ -1585,7 +1587,6 @@ gst_xvimagesink_setcaps (GstBaseSink * bsink, GstCaps * caps)
   gint video_width, video_height;
   gint video_par_n, video_par_d;        /* video's PAR */
   gint display_par_n, display_par_d;    /* display's PAR */
-  GValue display_ratio = { 0, };        /* display w/h ratio */
   const GValue *caps_par;
   const GValue *fps;
   gint num, den;
@@ -1626,9 +1627,7 @@ gst_xvimagesink_setcaps (GstBaseSink * bsink, GstCaps * caps)
 
   /* get aspect ratio from caps if it's present, and
    * convert video width and height to a display width and height
-   * using wd / hd = wv / hv * PARv / PARd
-   * the ratio wd / hd will be stored in display_ratio */
-  g_value_init (&display_ratio, GST_TYPE_FRACTION);
+   * using wd / hd = wv / hv * PARv / PARd */
 
   /* get video's PAR */
   caps_par = gst_structure_get_value (structure, "pixel-aspect-ratio");
@@ -1648,12 +1647,14 @@ gst_xvimagesink_setcaps (GstBaseSink * bsink, GstCaps * caps)
     display_par_d = 1;
   }
 
-  gst_value_set_fraction (&display_ratio,
-      video_width * video_par_n * display_par_d,
-      video_height * video_par_d * display_par_n);
+  if (!gst_video_calculate_display_ratio (&num, &den, video_width,
+          video_height, video_par_n, video_par_d, display_par_n,
+          display_par_d)) {
+    GST_ELEMENT_ERROR (xvimagesink, CORE, NEGOTIATION, (NULL),
+        ("Error calculating the output display ratio of the video."));
+    return FALSE;
+  }
 
-  num = gst_value_get_fraction_numerator (&display_ratio);
-  den = gst_value_get_fraction_denominator (&display_ratio);
   GST_DEBUG_OBJECT (xvimagesink,
       "video width/height: %dx%d, calculated display ratio: %d/%d",
       video_width, video_height, num, den);
@@ -1686,8 +1687,13 @@ gst_xvimagesink_setcaps (GstBaseSink * bsink, GstCaps * caps)
   }
 
   /* Creating our window and our image with the display size in pixels */
-  g_assert (GST_VIDEO_SINK_WIDTH (xvimagesink) > 0);
-  g_assert (GST_VIDEO_SINK_HEIGHT (xvimagesink) > 0);
+  if (GST_VIDEO_SINK_WIDTH (xvimagesink) <= 0 ||
+      GST_VIDEO_SINK_HEIGHT (xvimagesink) <= 0) {
+    GST_ELEMENT_ERROR (xvimagesink, CORE, NEGOTIATION, (NULL),
+        ("Error calculating the output display ratio of the video."));
+    return FALSE;
+  }
+
   if (!xvimagesink->xwindow)
     xvimagesink->xwindow = gst_xvimagesink_xwindow_new (xvimagesink,
         GST_VIDEO_SINK_WIDTH (xvimagesink),
diff --git a/tests/check/Makefile.am b/tests/check/Makefile.am
index 64e92a42..07db7c49 100644
--- a/tests/check/Makefile.am
+++ b/tests/check/Makefile.am
@@ -45,6 +45,7 @@ check_PROGRAMS = $(check_vorbis) \
 	generic/clock-selection \
 	generic/states \
 	libs/cddabasesrc \
+	libs/video \
         pipelines/simple-launch-lines
 
 # TORTURE_TO_FIX = \
@@ -54,6 +55,7 @@ VALGRIND_TO_FIX = \
 	elements/audioresample \
 	generic/states \
 	libs/cddabasesrc \
+	libs/video \
         pipelines/simple-launch-lines
 
 # these tests don't even pass
@@ -75,4 +77,9 @@ libs_cddabasesrc_CFLAGS = \
 	-I$(top_srcdir)/gst-libs \
 	$(CFLAGS) $(AM_CFLAGS)
 
+libs_video_LDADD = \
+	$(top_builddir)/gst-libs/gst/video/libgstvideo-@GST_MAJORMINOR@.la \
+	$(LDADD)
+libs_video_CFLAGS = -I$(top_srcdir)/gst-libs $(CFLAGS) $(AM_CFLAGS)
+
 EXTRA_DIST = gst-plugins-base.supp
diff --git a/tests/check/libs/video.c b/tests/check/libs/video.c
new file mode 100644
index 00000000..ca281a23
--- /dev/null
+++ b/tests/check/libs/video.c
@@ -0,0 +1,85 @@
+/* GStreamer
+ *
+ * unit test for video
+ *
+ * Copyright (C) <2006> Jan Schmidt <thaytan@mad.scientist.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <unistd.h>
+
+#include <gst/check/gstcheck.h>
+
+#include <gst/video/video.h>
+#include <string.h>
+
+GST_START_TEST (test_dar_calc)
+{
+  guint display_ratio_n, display_ratio_d;
+
+  /* Ensure that various Display Ratio calculations are correctly done */
+  /* video 768x576, par 16/15, display par 16/15 = 4/3 */
+  fail_unless (gst_video_calculate_display_ratio (&display_ratio_n,
+          &display_ratio_d, 768, 576, 16, 15, 16, 15));
+  fail_unless (display_ratio_n == 4 && display_ratio_d == 3);
+
+  /* video 720x480, par 32/27, display par 1/1 = 16/9 */
+  fail_unless (gst_video_calculate_display_ratio (&display_ratio_n,
+          &display_ratio_d, 720, 480, 32, 27, 1, 1));
+  fail_unless (display_ratio_n == 16 && display_ratio_d == 9);
+
+  /* video 360x288, par 533333/500000, display par 16/15 = 
+   * dar 1599999/1600000 */
+  fail_unless (gst_video_calculate_display_ratio (&display_ratio_n,
+          &display_ratio_d, 360, 288, 533333, 500000, 16, 15));
+  fail_unless (display_ratio_n == 1599999 && display_ratio_d == 1280000);
+}
+
+GST_END_TEST;
+
+Suite *
+video_suite (void)
+{
+  Suite *s = suite_create ("video support library");
+  TCase *tc_chain = tcase_create ("general");
+
+  suite_add_tcase (s, tc_chain);
+  tcase_add_test (tc_chain, test_dar_calc);
+
+  return s;
+}
+
+int
+main (int argc, char **argv)
+{
+  int nf;
+
+  Suite *s = video_suite ();
+  SRunner *sr = srunner_create (s);
+
+  gst_check_init (&argc, &argv);
+
+  srunner_run_all (sr, CK_NORMAL);
+  nf = srunner_ntests_failed (sr);
+  srunner_free (sr);
+
+  return nf;
+}