[libcamera-devel] [RFC PATCH 1/6] libcamera: Add fraction.h

[Laurent Pinchart]

Hi David,

On Fri, Mar 19, 2021 at 09:43:48AM +0000, David Plowman wrote:
> 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!

It's more than alright, you've provided very valuable feedback on a wide
variety of topics, and I'd be very happy to see you sharing your opinion
on anything related to libcamera :-)

> On Fri, 19 Mar 2021 at 00:15, Laurent Pinchart wrote:
> > 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.

I share your opinion on this topic.

> > >> 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!
> 
> > > > 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