[libcamera-devel] libcamera: pipeline: simple: Use std::make_tuple()

Message ID 20200714233325.14951-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit ed1b290aab18cfd9bf97a4e85f79b4d65d1906ca
Headers show
Series
  • [libcamera-devel] libcamera: pipeline: simple: Use std::make_tuple()
Related show

Commit Message

Laurent Pinchart July 14, 2020, 11:33 p.m. UTC
Prior to N4387 ([1]), the std::tuple::tuple(const Types&...) constructor
was unconditionally explicit. gcc 5 doesn't implement N4387, leading to
a compilation error in the simple pipeline handler. Use
std::make_tuple() to fix it.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/converter.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Niklas Söderlund July 15, 2020, 6:20 a.m. UTC | #1
Hi Laurent,

Thanks for your work.

On 2020-07-15 02:33:25 +0300, Laurent Pinchart wrote:
> Prior to N4387 ([1]), the std::tuple::tuple(const Types&...) constructor
> was unconditionally explicit. gcc 5 doesn't implement N4387, leading to
> a compilation error in the simple pipeline handler. Use
> std::make_tuple() to fix it.
> 
> [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Maybe it's time we considered to drop support for gcc5 and possibly even 
gcc6? But that is a topic for a another thread,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/pipeline/simple/converter.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index dc7c046329f1..75fb297ebd58 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -271,9 +271,9 @@ SimpleConverter::strideAndFrameSize(const Size &size,
>  
>  	int ret = m2m_->capture()->tryFormat(&format);
>  	if (ret < 0)
> -		return { 0, 0 };
> +		return std::make_tuple(0, 0);
>  
> -	return { format.planes[0].bpl, format.planes[0].size };
> +	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
>  }
>  
>  } /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 15, 2020, 9:35 a.m. UTC | #2
Hi Niklas,

On Wed, Jul 15, 2020 at 08:20:08AM +0200, Niklas Söderlund wrote:
> On 2020-07-15 02:33:25 +0300, Laurent Pinchart wrote:
> > Prior to N4387 ([1]), the std::tuple::tuple(const Types&...) constructor
> > was unconditionally explicit. gcc 5 doesn't implement N4387, leading to
> > a compilation error in the simple pipeline handler. Use
> > std::make_tuple() to fix it.
> > 
> > [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Maybe it's time we considered to drop support for gcc5 and possibly even 
> gcc6? But that is a topic for a another thread,

I wouldn't be against that.

> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> > ---
> >  src/libcamera/pipeline/simple/converter.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > index dc7c046329f1..75fb297ebd58 100644
> > --- a/src/libcamera/pipeline/simple/converter.cpp
> > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > @@ -271,9 +271,9 @@ SimpleConverter::strideAndFrameSize(const Size &size,
> >  
> >  	int ret = m2m_->capture()->tryFormat(&format);
> >  	if (ret < 0)
> > -		return { 0, 0 };
> > +		return std::make_tuple(0, 0);
> >  
> > -	return { format.planes[0].bpl, format.planes[0].size };
> > +	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
> >  }
> >  
> >  } /* namespace libcamera */
Kieran Bingham July 16, 2020, 2:06 p.m. UTC | #3
Hi Niklas, Laurent,

On 15/07/2020 10:35, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Wed, Jul 15, 2020 at 08:20:08AM +0200, Niklas Söderlund wrote:
>> On 2020-07-15 02:33:25 +0300, Laurent Pinchart wrote:
>>> Prior to N4387 ([1]), the std::tuple::tuple(const Types&...) constructor
>>> was unconditionally explicit. gcc 5 doesn't implement N4387, leading to
>>> a compilation error in the simple pipeline handler. Use
>>> std::make_tuple() to fix it.
>>>
>>> [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Maybe it's time we considered to drop support for gcc5 and possibly even 
>> gcc6? But that is a topic for a another thread,
> 
> I wouldn't be against that.

I can't recall if we discussed on IRC or a thread, but indeed, if a
distribution has a kernel older than we can support, then I don't think
we need to support the build on that system either.

- Ubuntu only brought in kernel v5.0 at 18.04.3, and 18.04 ships with
gcc v7.

- On Debian, Stretch(9) uses kernel v4.14, and buster(10) has v5.4.
Buster ships with GCC v8.

- Fedora picks up a v5.0 kernel in Fedora 30, which ships gcc v9.



So I don't foresee much reason to keep gcc 5/6 support around ...
Does anyone know of a distribution that uses a kernel >= 5.0 which ships
gcc <= 6?



> 
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>
>>> ---
>>>  src/libcamera/pipeline/simple/converter.cpp | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
>>> index dc7c046329f1..75fb297ebd58 100644
>>> --- a/src/libcamera/pipeline/simple/converter.cpp
>>> +++ b/src/libcamera/pipeline/simple/converter.cpp
>>> @@ -271,9 +271,9 @@ SimpleConverter::strideAndFrameSize(const Size &size,
>>>  
>>>  	int ret = m2m_->capture()->tryFormat(&format);
>>>  	if (ret < 0)
>>> -		return { 0, 0 };
>>> +		return std::make_tuple(0, 0);
>>>  
>>> -	return { format.planes[0].bpl, format.planes[0].size };
>>> +	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);

GCC support aside, I don't see anything wrong here in this patch though
if you still want to integrate it...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>>>  }
>>>  
>>>  } /* namespace libcamera */
>
Laurent Pinchart July 16, 2020, 2:39 p.m. UTC | #4
Hi Kieran,

On Thu, Jul 16, 2020 at 03:06:08PM +0100, Kieran Bingham wrote:
> On 15/07/2020 10:35, Laurent Pinchart wrote:
> > On Wed, Jul 15, 2020 at 08:20:08AM +0200, Niklas Söderlund wrote:
> >> On 2020-07-15 02:33:25 +0300, Laurent Pinchart wrote:
> >>> Prior to N4387 ([1]), the std::tuple::tuple(const Types&...) constructor
> >>> was unconditionally explicit. gcc 5 doesn't implement N4387, leading to
> >>> a compilation error in the simple pipeline handler. Use
> >>> std::make_tuple() to fix it.
> >>>
> >>> [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> Maybe it's time we considered to drop support for gcc5 and possibly even 
> >> gcc6? But that is a topic for a another thread,
> > 
> > I wouldn't be against that.
> 
> I can't recall if we discussed on IRC or a thread, but indeed, if a
> distribution has a kernel older than we can support, then I don't think
> we need to support the build on that system either.
> 
> - Ubuntu only brought in kernel v5.0 at 18.04.3, and 18.04 ships with
> gcc v7.
> 
> - On Debian, Stretch(9) uses kernel v4.14, and buster(10) has v5.4.
> Buster ships with GCC v8.
> 
> - Fedora picks up a v5.0 kernel in Fedora 30, which ships gcc v9.
> 
> So I don't foresee much reason to keep gcc 5/6 support around ...
> Does anyone know of a distribution that uses a kernel >= 5.0 which ships
> gcc <= 6?

You've started a separate mail thread on that topic, so let's discuss it
there.

> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>
> >>> ---
> >>>  src/libcamera/pipeline/simple/converter.cpp | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> >>> index dc7c046329f1..75fb297ebd58 100644
> >>> --- a/src/libcamera/pipeline/simple/converter.cpp
> >>> +++ b/src/libcamera/pipeline/simple/converter.cpp
> >>> @@ -271,9 +271,9 @@ SimpleConverter::strideAndFrameSize(const Size &size,
> >>>  
> >>>  	int ret = m2m_->capture()->tryFormat(&format);
> >>>  	if (ret < 0)
> >>> -		return { 0, 0 };
> >>> +		return std::make_tuple(0, 0);
> >>>  
> >>> -	return { format.planes[0].bpl, format.planes[0].size };
> >>> +	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
> 
> GCC support aside, I don't see anything wrong here in this patch though
> if you still want to integrate it...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >>>  }
> >>>  
> >>>  } /* namespace libcamera */

Patch

diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
index dc7c046329f1..75fb297ebd58 100644
--- a/src/libcamera/pipeline/simple/converter.cpp
+++ b/src/libcamera/pipeline/simple/converter.cpp
@@ -271,9 +271,9 @@  SimpleConverter::strideAndFrameSize(const Size &size,
 
 	int ret = m2m_->capture()->tryFormat(&format);
 	if (ret < 0)
-		return { 0, 0 };
+		return std::make_tuple(0, 0);
 
-	return { format.planes[0].bpl, format.planes[0].size };
+	return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
 }
 
 } /* namespace libcamera */