[libcamera-devel] [RFC PATCH 1/6] libcamera: Add fraction.h
David Plowman
david.plowman at raspberrypi.com
Fri Mar 19 10:43:48 CET 2021
Hi everyone
Thanks for this interesting proposal. I don't normally venture too many
opinions on stuff that doesn't really affect the Pi, but I did have just a
couple of comments on this one, if that's all right!
On Fri, 19 Mar 2021 at 00:15, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Marian,
>
> On Wed, Mar 17, 2021 at 06:19:26PM +0100, Marian Cichy wrote:
> > On 3/17/21 2:48 PM, Kieran Bingham wrote:
> > > On 17/03/2021 12:41, Marian Cichy wrote:
> > >> On 3/17/21 12:33 PM, Kieran Bingham wrote:
> > >>> On 16/03/2021 15:52, Marian Cichy wrote:
> > >>>> Add a new class that represents a fraction. A structure like this is
> > >>>> also often used in linux camera drivers, e.g. v4l2_fract to
> represent a
> > >>>> frame interval or in applications like Gstreamer to represent frame
> > >>>> rates.
> > >>>>
> > >>>> Adding this class helps to interface frame intervals and frame
> > >>>> rates in video streams.
> > >>> This indeed would be helpful in any area that handles time.
> > >>>
> > >>> I think this is normally called a rational though?
>
1. I think "fraction" is not an ideal name. The very fact that we've seen
some discussion about what, exactly, a "fraction" is kind of demonstrates
the problem.
Personally, I would be inclined to go with "rational". It has a cast iron
and straightforward mathematical definition. (Numbers of the form p/q,
where p and q are both integers, q != 0).
2. Following the idea of using rationals, the numerator would need to
become signed.
In particular, this makes rationals closed under the usual arithmetic
operations (+ - * /). There may be no plans to overload them, but you could
imagine, in a C++ world, someone wanting to do that and finding the lack of
negative numbers rather awkward! With rationals, you at least get to do
whatever calculation you want, and test if the result is ">= 0" or not at
the end.
> >> Well .. this is probably mathematical nit-picking, but a fraction is
> > >> always a construct that consists of two integers, numerator and
> > >> denominator. A rational number is any number that *can* be displayed
> as
> > >> a fraction. For example, "5" is in the set of rational numbers,
> because
> > >> we can display it as the fraction "5/1".
> > > Indeed, and wouldn't we use such a representation in the case of long
> > > exposures if we use this for frame rates?
> >
> > But as the frame rate is configurable to be, for example 30 or 29.97, it
> > is only sometimes representable by single integers but always by
> > fractions. Gstreamer also reports 30/1 for a frame rate of 30.
>
> This would be true if the frame rate was really 30 or 29.97 fps, but
> that's rarely the case in practice with cameras. For instance, there's
> an Intel camera HAL implementation that produces ~29.94 fps, and reports
> an exact 30 fps to Android as the Android camera framework wants at
> least 29.97 fps for the corresponding use case.
>
> I'm not entirely opposed to using fractions, but we need to consider the
> implications carefully, looking at the big picture. It's a non-trivial
> topic, and lots of assumptions from the TV world don't apply as-is to
> cameras.
>
> > > Isn't 5/1 the frame rate for a 5 second exposure?
> >
> > 5/1 is the frame rate of 0.2 seconds exposure
>
> For 0.2s of frame duration. The exposure time can be (and usually is)
> shorter.
>
> > 5 frames per 1 second
> >
> > 5/1 is the frame interval of 5 seconds exposure
> >
> > 5 seconds interval for 1 frame
> >
> > I know, numbers are confusing :-(
> >
> > > However I don't think 5/1 could easily be called a fraction...
> > >
> > > <Ok, after some googling, it looks like they can be called 'improper
> > > fractions'>
> > >
> > >> Also, since it is called v4l2_fract in V4L2 and GST_FRACTION in
> > >> Gstreamer, I went for consistency in the linux media universe.
> > >
>
I appreciate that "rationals" don't 100% match the v4l2_fract any more, but
the inability to represent some of those rationals where you need full
32-bit positive denominators seems negligible to me. However, I'd be
delighted to be educated on that if anyone can think of any examples!
Thanks and best regards
David
> > > Ah, yes I see that reference later in the series now which I hadn't
> before.
> > >
> > >>> We're really building up a math library in here aren't we with our
> > >>> sizes, geometry, and number classes...
> > >>>
> > >>>> Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> > >>>> ---
> > >>>> include/libcamera/fraction.h | 34 ++++++++++++++++++++
> > >>>> src/libcamera/fraction.cpp | 60
> ++++++++++++++++++++++++++++++++++++
> > >>>> src/libcamera/meson.build | 1 +
> > >>>> 3 files changed, 95 insertions(+)
> > >>>> create mode 100644 include/libcamera/fraction.h
> > >>>> create mode 100644 src/libcamera/fraction.cpp
> > >>>>
> > >>>> diff --git a/include/libcamera/fraction.h
> b/include/libcamera/fraction.h
> > >>>> new file mode 100644
> > >>>> index 00000000..aa6a1abb
> > >>>> --- /dev/null
> > >>>> +++ b/include/libcamera/fraction.h
> > >>>> @@ -0,0 +1,34 @@
> > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>>> +/*
> > >>>> + * Copyright (C) 2021, Pengutronix, Marian Cichy
> > >>>> <entwicklung at pengutronix.de>
> > >>>> + *
> > >>>> + * fraction.h - A fraction consisting of two integers
> > >>>> + */
> > >>>> +
> > >>>> +#ifndef __LIBCAMERA_FRACTION_H__
> > >>>> +#define __LIBCAMERA_FRACTION_H__
> > >>>> +
> > >>>> +namespace libcamera {
> > >>>> +
> > >>>> +class Fraction {
> > >>>> +public:
> > >>>> + constexpr Fraction() :
> > >>>> + numerator(0), denominator(0)
> > >>>> + {
> > >>>> + }
> > >>>> +
> > >>>> + constexpr Fraction(unsigned int num, unsigned int den) :
> > >>>> + numerator(num), denominator(den)
> > >>>> + {
> > >>>> + }
> > >>>> +
> > >>>> + unsigned int numerator;
> > >>>> + unsigned int denominator;
> > >>> Generically - Fractions/(Rational) numbers could be negative.
> > >>> But given our use cases, I presume we wouldn't expect negative
> values ?
> > >>>
> > >>> Well ... not unless we start needing to do things like clock recovery
> > >>> .... I don't think we'll need to do anything like that ...
> > >>>
> > >>> Would you expect further math functions in here to be able to add /
> > >>> multiple two fractions/rational numbers together?
> > >>>
> > >>> In fact - I wonder if this would also tie into the geometry functions
> > >>> for scaling or such?
> > >>>
> > >>> Size(640, 480) * Rational(1/2) == Size(320, 240);
> > >>>
> > >>> I'm not saying the extra functionality possible here is required for
> > >>> this patch though - just exploring what the goals would be for this
> > >>> class.
> > >>>
> > >>> --
> > >>> Kieran
> > >>>
> > >>>
> > >>>
> > >>>> +
> > >>>> + const std::string toString() const;
> > >>>> +};
> > >>>> +
> > >>>> +} /* namespace libcamera */
> > >>>> +
> > >>>> +#endif /* __LIBCAMERA_FRACTION_H__ */
> > >>>> diff --git a/src/libcamera/fraction.cpp b/src/libcamera/fraction.cpp
> > >>>> new file mode 100644
> > >>>> index 00000000..76c373aa
> > >>>> --- /dev/null
> > >>>> +++ b/src/libcamera/fraction.cpp
> > >>>> @@ -0,0 +1,60 @@
> > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>>> +/*
> > >>>> + * Copyright (C) 2021, Pengutronix, Marian Cichy
> > >>>> <entwicklung at pengutronix.de>
> > >>>> + *
> > >>>> + * fraction.h - A fraction consisting of two integers
> > >>>> + */
> > >>>> +
> > >>>> +#include <string>
> > >>>> +#include <sstream>
> > >>>> +
> > >>>> +#include <libcamera/fraction.h>
> > >>>> +
> > >>>> +/**
> > >>>> + * \file fraction.h
> > >>>> + * \brief A fraction consisting of two integers.
> > >>>> + */
> > >>>> +
> > >>>> +namespace libcamera {
> > >>>> +
> > >>>> +/**
> > >>>> + * \class Fraction
> > >>>> + * \brief Represents a fraction with a nominator and a denominator.
> > >>>> + */
> > >>>> +
> > >>>> +/**
> > >>>> + * \fn Fraction::Fraction()
> > >>>> + * \brief Construct a Fraction with value 0/0. This should be
> > >>>> interpreted
> > >>>> + * as invalid or not-used Fraction.
> > >>>> + */
> > >>>> +
> > >>>> +/**
> > >>>> + * \fn Fraction::Fraction(unsigned int num, unsigned int den)
> > >>>> + * \brief Construct a Fraction with value n/d.
> > >>>> + */
> > >>>> +
> > >>>> +/**
> > >>>> + * \var Fraction::numerator
> > >>>> + * \brief The numerator of the fraction.
> > >>>> + */
> > >>>> +
> > >>>> +/**
> > >>>> + * \var Fraction::denominator
> > >>>> + * \brief The denominator of the fraction.
> > >>>> + */
> > >>>> +
> > >>>> +/**
> > >>>> + * \brief Assemble and return a string describing the fraction
> > >>>> + * \return A string describing the fraction.
> > >>>> + */
> > >>>> +const std::string Fraction::toString() const
> > >>>> +{
> > >>>> + std::stringstream ss;
> > >>>> +
> > >>>> + ss << numerator << "/" << denominator;
> > >>>> +
> > >>>> + return ss.str();
> > >>>> +}
> > >>>> +
> > >>>> +} /* namespace libcamera */
> > >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > >>>> index 815629db..7927481f 100644
> > >>>> --- a/src/libcamera/meson.build
> > >>>> +++ b/src/libcamera/meson.build
> > >>>> @@ -22,6 +22,7 @@ libcamera_sources = files([
> > >>>> 'file.cpp',
> > >>>> 'file_descriptor.cpp',
> > >>>> 'formats.cpp',
> > >>>> + 'fraction.cpp',
> > >>>> 'framebuffer_allocator.cpp',
> > >>>> 'geometry.cpp',
> > >>>> 'ipa_controls.cpp',
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210319/28bef905/attachment-0001.htm>
More information about the libcamera-devel
mailing list