| Message ID | 20230214121448.54918-1-ecurtin@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, (CC'ing Barnabás) Thank you for the patch. On Tue, Feb 14, 2023 at 12:14:49PM +0000, Eric Curtin via libcamera-devel wrote: > ../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > | ^~~~ > ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)' > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > | ^ > cc1plus: all warnings being treated as errors This seems to be a false positive from an unreleased gcc version. Could you report the issue to upstream gcc ? I'd like to get it fixed if it's indeed a false positive, or at least get confirmation from them that they won't fix it. > Co-developed-by: Khem Raj <raj.khem@gmail.com> > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > --- > Changes in v2: > > - Added const > - Made patch mergeable by accounting for new directory structure > --- > src/apps/cam/file_sink.cpp | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > index b32aad24..d41569c9 100644 > --- a/src/apps/cam/file_sink.cpp > +++ b/src/apps/cam/file_sink.cpp > @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > } > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > - const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > - > + const unsigned int bytesused = buffer->metadata().planes()[i].bytesused; > Span<uint8_t> data = image->data(i); > - unsigned int length = std::min<unsigned int>(meta.bytesused, data.size()); > + const unsigned int length = std::min<unsigned int>(bytesused, data.size()); > > - if (meta.bytesused > data.size()) > - std::cerr << "payload size " << meta.bytesused > + if (bytesused > data.size()) > + std::cerr << "payload size " << bytesused > << " larger than plane size " << data.size() > << std::endl; >
On Tue, 14 Feb 2023 at 13:24, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Eric, > > (CC'ing Barnabás) > > Thank you for the patch. > > On Tue, Feb 14, 2023 at 12:14:49PM +0000, Eric Curtin via libcamera-devel wrote: > > ../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > | ^~~~ > > ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)' > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > | ^ > > cc1plus: all warnings being treated as errors > > This seems to be a false positive from an unreleased gcc version. Could > you report the issue to upstream gcc ? I'd like to get it fixed if it's Reported here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532 I added another poke. > indeed a false positive, or at least get confirmation from them that > they won't fix it. > > > Co-developed-by: Khem Raj <raj.khem@gmail.com> > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > --- > > Changes in v2: > > > > - Added const > > - Made patch mergeable by accounting for new directory structure > > --- > > src/apps/cam/file_sink.cpp | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > > index b32aad24..d41569c9 100644 > > --- a/src/apps/cam/file_sink.cpp > > +++ b/src/apps/cam/file_sink.cpp > > @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > > } > > > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > - const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > - > > + const unsigned int bytesused = buffer->metadata().planes()[i].bytesused; > > Span<uint8_t> data = image->data(i); > > - unsigned int length = std::min<unsigned int>(meta.bytesused, data.size()); > > + const unsigned int length = std::min<unsigned int>(bytesused, data.size()); > > > > - if (meta.bytesused > data.size()) > > - std::cerr << "payload size " << meta.bytesused > > + if (bytesused > data.size()) > > + std::cerr << "payload size " << bytesused > > << " larger than plane size " << data.size() > > << std::endl; > > > > -- > Regards, > > Laurent Pinchart >
Quoting Eric Curtin via libcamera-devel (2023-02-14 13:34:11) > On Tue, 14 Feb 2023 at 13:24, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Eric, > > > > (CC'ing Barnabás) > > > > Thank you for the patch. > > > > On Tue, Feb 14, 2023 at 12:14:49PM +0000, Eric Curtin via libcamera-devel wrote: > > > ../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > | ^~~~ > > > ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)' > > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > | ^ > > > cc1plus: all warnings being treated as errors > > > > This seems to be a false positive from an unreleased gcc version. Could > > you report the issue to upstream gcc ? I'd like to get it fixed if it's > > Reported here: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532 > > I added another poke. > > > indeed a false positive, or at least get confirmation from them that > > they won't fix it. > > Let's add this here then: Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532 > > > Co-developed-by: Khem Raj <raj.khem@gmail.com> > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > --- > > > Changes in v2: > > > > > > - Added const > > > - Made patch mergeable by accounting for new directory structure > > > --- > > > src/apps/cam/file_sink.cpp | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > > > index b32aad24..d41569c9 100644 > > > --- a/src/apps/cam/file_sink.cpp > > > +++ b/src/apps/cam/file_sink.cpp > > > @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > > > } > > > > > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) { I'd be tempted to add some sort of explainer here in a comment, but maybe that's overkill if the bug does get fixed before GCC-13 is released...? > > > - const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > - > > > + const unsigned int bytesused = buffer->metadata().planes()[i].bytesused; > > > Span<uint8_t> data = image->data(i); > > > - unsigned int length = std::min<unsigned int>(meta.bytesused, data.size()); > > > + const unsigned int length = std::min<unsigned int>(bytesused, data.size()); > > > > > > - if (meta.bytesused > data.size()) > > > - std::cerr << "payload size " << meta.bytesused > > > + if (bytesused > data.size()) > > > + std::cerr << "payload size " << bytesused Otherwise, the change itself seems fair, and does the 'same thing'... so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > << " larger than plane size " << data.size() > > > << std::endl; > > > > > > > -- > > Regards, > > > > Laurent Pinchart > > >
Quoting Kieran Bingham (2023-02-18 23:00:15) > Quoting Eric Curtin via libcamera-devel (2023-02-14 13:34:11) > > On Tue, 14 Feb 2023 at 13:24, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Eric, > > > > > > (CC'ing Barnabás) > > > > > > Thank you for the patch. > > > > > > On Tue, Feb 14, 2023 at 12:14:49PM +0000, Eric Curtin via libcamera-devel wrote: > > > > ../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > > | ^~~~ > > > > ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)' > > > > 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > > | ^ > > > > cc1plus: all warnings being treated as errors > > > > > > This seems to be a false positive from an unreleased gcc version. Could > > > you report the issue to upstream gcc ? I'd like to get it fixed if it's > > > > Reported here: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532 > > > > I added another poke. > > > > > indeed a false positive, or at least get confirmation from them that > > > they won't fix it. > > > > > Let's add this here then: > > Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532 So it seems from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c13 that the GCC-13 'bug fix' fixed the minimal test case, but possibly not the false positive in libcamera. Can anyone else test to confirm? -- Kieran > > > > > Co-developed-by: Khem Raj <raj.khem@gmail.com> > > > > Signed-off-by: Eric Curtin <ecurtin@redhat.com> > > > > --- > > > > Changes in v2: > > > > > > > > - Added const > > > > - Made patch mergeable by accounting for new directory structure > > > > --- > > > > src/apps/cam/file_sink.cpp | 9 ++++----- > > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > > > > index b32aad24..d41569c9 100644 > > > > --- a/src/apps/cam/file_sink.cpp > > > > +++ b/src/apps/cam/file_sink.cpp > > > > @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > > > > } > > > > > > > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) { > > I'd be tempted to add some sort of explainer here in a comment, but > maybe that's overkill if the bug does get fixed before GCC-13 is > released...? > > > > > - const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; > > > > - > > > > + const unsigned int bytesused = buffer->metadata().planes()[i].bytesused; > > > > Span<uint8_t> data = image->data(i); > > > > - unsigned int length = std::min<unsigned int>(meta.bytesused, data.size()); > > > > + const unsigned int length = std::min<unsigned int>(bytesused, data.size()); > > > > > > > > - if (meta.bytesused > data.size()) > > > > - std::cerr << "payload size " << meta.bytesused > > > > + if (bytesused > data.size()) > > > > + std::cerr << "payload size " << bytesused > > Otherwise, the change itself seems fair, and does the 'same thing'... > so: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > << " larger than plane size " << data.size() > > > > << std::endl; > > > > > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > > > >
diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp index b32aad24..d41569c9 100644 --- a/src/apps/cam/file_sink.cpp +++ b/src/apps/cam/file_sink.cpp @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, } for (unsigned int i = 0; i < buffer->planes().size(); ++i) { - const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; - + const unsigned int bytesused = buffer->metadata().planes()[i].bytesused; Span<uint8_t> data = image->data(i); - unsigned int length = std::min<unsigned int>(meta.bytesused, data.size()); + const unsigned int length = std::min<unsigned int>(bytesused, data.size()); - if (meta.bytesused > data.size()) - std::cerr << "payload size " << meta.bytesused + if (bytesused > data.size()) + std::cerr << "payload size " << bytesused << " larger than plane size " << data.size() << std::endl;
../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference] 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; | ^~~~ ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)' 92 | const FrameMetadata::Plane &meta = buffer->metadata().planes()[i]; | ^ cc1plus: all warnings being treated as errors Co-developed-by: Khem Raj <raj.khem@gmail.com> Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- Changes in v2: - Added const - Made patch mergeable by accounting for new directory structure --- src/apps/cam/file_sink.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)