| Message ID | 20201028085726.2983867-1-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro-san, On 28/10/2020 08:57, Hirokazu Honda wrote: > std::map::at() searches std::map by the given key. The commit > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to > accessing the first element of the map, but actually access the > element whose key is nullptr. This causes the crash because the map > doesn't have the element with nullptr. This fixes the issue by > replacing the std::map::at() operation by std::map::begin(). > Ayeee - I'm sorry - It looks like I certainly messed up something there. Before pushing, I ran that series through our unit-tests, but of course that doesn't cover src/android, and I had obviously neglected to further test the android layer. The clue that I should have done so is in the prefix of the patches, so I'm sorry that I missed that. Even the code it came from was correct: - FrameBuffer *buffer = buffers.begin()->second; - resultMetadata = getResultMetadata(descriptor->frameNumber_, - buffer->metadata().timestamp); + uint64_t timestamp = buffers.at(0)->metadata().timestamp; + resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); So your fix is certainly better. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> It makes me wonder if the integration processes needs to be able to cover more use-cases, but it's not going to be easy to automate testing of the android layer from a normal build currently. We'd have to mock up the Android HAL calls ... hrm. .. not impossible, but I wonder what the best way to do all that would be (without requiring a full CTS). > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_device.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index ca60f51..ead8a43 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request) > * It might be appropriate to return a 'correct' (as determined by > * pipeline handlers) timestamp in the Request itself. > */ > - uint64_t timestamp = buffers.at(0)->metadata().timestamp; > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp; > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); > > /* Handle any JPEG compression. */ > -- > 2.29.0.rc2.309.g374f81d7ae-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote: > Hi Hiro-san, > > On 28/10/2020 08:57, Hirokazu Honda wrote: > > std::map::at() searches std::map by the given key. The commit > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to > > accessing the first element of the map, but actually access the > > element whose key is nullptr. This causes the crash because the map > > doesn't have the element with nullptr. This fixes the issue by > > replacing the std::map::at() operation by std::map::begin(). > > > > Ayeee - I'm sorry - It looks like I certainly messed up something there. > > Before pushing, I ran that series through our unit-tests, but of course > that doesn't cover src/android, and I had obviously neglected to further > test the android layer. The clue that I should have done so is in the > prefix of the patches, so I'm sorry that I missed that. > > > Even the code it came from was correct: > > - FrameBuffer *buffer = buffers.begin()->second; > - resultMetadata = getResultMetadata(descriptor->frameNumber_, > - buffer->metadata().timestamp); > + uint64_t timestamp = buffers.at(0)->metadata().timestamp; > + resultMetadata = getResultMetadata(descriptor->frameNumber_, > timestamp); > > So your fix is certainly better. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > It makes me wonder if the integration processes needs to be able to > cover more use-cases, but it's not going to be easy to automate testing > of the android layer from a normal build currently. > > We'd have to mock up the Android HAL calls ... hrm. .. not impossible, > but I wonder what the best way to do all that would be (without > requiring a full CTS). We could have basic smoketests, but we're missing the infrastructure to run them automatically on a real (or even virtual) device at the moment. Clearly something we'll have to address. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> And we really need to move the timestamp from the buffers to the request. > > --- > > src/android/camera_device.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index ca60f51..ead8a43 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request) > > * It might be appropriate to return a 'correct' (as determined by > > * pipeline handlers) timestamp in the Request itself. > > */ > > - uint64_t timestamp = buffers.at(0)->metadata().timestamp; > > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp; > > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); > > > > /* Handle any JPEG compression. */
On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote: > > Hi Hiro-san, > > > > On 28/10/2020 08:57, Hirokazu Honda wrote: > > > std::map::at() searches std::map by the given key. The commit > > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to > > > accessing the first element of the map, but actually access the > > > element whose key is nullptr. This causes the crash because the map > > > doesn't have the element with nullptr. This fixes the issue by > > > replacing the std::map::at() operation by std::map::begin(). > > > > > > > Ayeee - I'm sorry - It looks like I certainly messed up something there. > > > > Before pushing, I ran that series through our unit-tests, but of course > > that doesn't cover src/android, and I had obviously neglected to further > > test the android layer. The clue that I should have done so is in the > > prefix of the patches, so I'm sorry that I missed that. > > > > > > Even the code it came from was correct: > > > > - FrameBuffer *buffer = buffers.begin()->second; > > - resultMetadata = getResultMetadata(descriptor->frameNumber_, > > - buffer->metadata().timestamp); > > + uint64_t timestamp = buffers.at(0)->metadata().timestamp; > > + resultMetadata = getResultMetadata(descriptor->frameNumber_, > > timestamp); > > > > So your fix is certainly better. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > It makes me wonder if the integration processes needs to be able to > > cover more use-cases, but it's not going to be easy to automate testing > > of the android layer from a normal build currently. > > > > We'd have to mock up the Android HAL calls ... hrm. .. not impossible, > > but I wonder what the best way to do all that would be (without > > requiring a full CTS). > > We could have basic smoketests, but we're missing the infrastructure to > run them automatically on a real (or even virtual) device at the moment. > Clearly something we'll have to address. > I honestly haven't run the unit tests. Where are unit tests for Android HAL adaptation layer? I test with the ChromeOS camera stack + libcamera. +1 to adding smoketests. A HAL client of doing a simple (still) capture + mock/fake libcamera classes should be nice to have. Best Regards, -Hiro > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > And we really need to move the timestamp from the buffers to the > request. > > > > --- > > > src/android/camera_device.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index ca60f51..ead8a43 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request) > > > * It might be appropriate to return a 'correct' (as determined by > > > * pipeline handlers) timestamp in the Request itself. > > > */ > > > - uint64_t timestamp = buffers.at(0)->metadata().timestamp; > > > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp; > > > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); > > > > > > /* Handle any JPEG compression. */ > > -- > Regards, > > Laurent Pinchart
Hi Hiro-san, On Thu, Oct 29, 2020 at 04:01:14PM +0900, Hirokazu Honda wrote: > On Wed, Oct 28, 2020 at 7:36 PM Laurent Pinchart wrote: > > On Wed, Oct 28, 2020 at 10:33:04AM +0000, Kieran Bingham wrote: > > > On 28/10/2020 08:57, Hirokazu Honda wrote: > > > > std::map::at() searches std::map by the given key. The commit > > > > e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to > > > > accessing the first element of the map, but actually access the > > > > element whose key is nullptr. This causes the crash because the map > > > > doesn't have the element with nullptr. This fixes the issue by > > > > replacing the std::map::at() operation by std::map::begin(). > > > > > > > > > > Ayeee - I'm sorry - It looks like I certainly messed up something there. > > > > > > Before pushing, I ran that series through our unit-tests, but of course > > > that doesn't cover src/android, and I had obviously neglected to further > > > test the android layer. The clue that I should have done so is in the > > > prefix of the patches, so I'm sorry that I missed that. > > > > > > > > > Even the code it came from was correct: > > > > > > - FrameBuffer *buffer = buffers.begin()->second; > > > - resultMetadata = getResultMetadata(descriptor->frameNumber_, > > > - buffer->metadata().timestamp); > > > + uint64_t timestamp = buffers.at(0)->metadata().timestamp; > > > + resultMetadata = getResultMetadata(descriptor->frameNumber_, > > > timestamp); > > > > > > So your fix is certainly better. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > It makes me wonder if the integration processes needs to be able to > > > cover more use-cases, but it's not going to be easy to automate testing > > > of the android layer from a normal build currently. > > > > > > We'd have to mock up the Android HAL calls ... hrm. .. not impossible, > > > but I wonder what the best way to do all that would be (without > > > requiring a full CTS). > > > > We could have basic smoketests, but we're missing the infrastructure to > > run them automatically on a real (or even virtual) device at the moment. > > Clearly something we'll have to address. > > I honestly haven't run the unit tests. Where are unit tests for > Android HAL adaptation layer? > I test with the ChromeOS camera stack + libcamera. We don't have custom unit tests for the camera HAL, we also rely on the Chrome OS camera tests, and CTS. We however have no infrastructure to automate this at the moment, so it's all manual, and doesn't get tested on every commit :-S That's something I would really like to fix, but of course we can't pause development for a month to put all the focus on test automation :-) > +1 to adding smoketests. > A HAL client of doing a simple (still) capture + mock/fake libcamera > classes should be nice to have. > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > And we really need to move the timestamp from the buffers to the > > request. > > > > > > --- > > > > src/android/camera_device.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index ca60f51..ead8a43 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request) > > > > * It might be appropriate to return a 'correct' (as determined by > > > > * pipeline handlers) timestamp in the Request itself. > > > > */ > > > > - uint64_t timestamp = buffers.at(0)->metadata().timestamp; > > > > + uint64_t timestamp = buffers.begin()->second->metadata().timestamp; > > > > resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); > > > > > > > > /* Handle any JPEG compression. */
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index ca60f51..ead8a43 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1525,7 +1525,7 @@ void CameraDevice::requestComplete(Request *request) * It might be appropriate to return a 'correct' (as determined by * pipeline handlers) timestamp in the Request itself. */ - uint64_t timestamp = buffers.at(0)->metadata().timestamp; + uint64_t timestamp = buffers.begin()->second->metadata().timestamp; resultMetadata = getResultMetadata(descriptor->frameNumber_, timestamp); /* Handle any JPEG compression. */
std::map::at() searches std::map by the given key. The commit e1f9fdb8a5bd096faa34d54804afa48d46d59b28 uses it with 0 to intend to accessing the first element of the map, but actually access the element whose key is nullptr. This causes the crash because the map doesn't have the element with nullptr. This fixes the issue by replacing the std::map::at() operation by std::map::begin(). Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_device.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.29.0.rc2.309.g374f81d7ae-goog