| Message ID | 20250929074706.420894-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-09-29 08:47:06) > Debayering is carried out on `ispWorkerThread_`. When stopping, the queued > work needs to be flushed or cancelled to ensure that the next time it starts, > it won't process stale data. So remove all messages targeting the `Debayer` > object on the worker thread. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/software_isp/software_isp.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index b7651b7d2..fdadf79e1 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -358,6 +358,7 @@ void SoftwareIsp::stop() > { > ispWorkerThread_.exit(); > ispWorkerThread_.wait(); > + ispWorkerThread_.removeMessages(debayer_.get()); As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ messages be dropped? Should we wrap a helper to the thread class that will - Exit the thread - Block until it's stopped - Remove all messages ? That seems like a common 'requirement' for a thread that could be restarted perhaps ? > > Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); > > -- > 2.51.0 >
Hi 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-09-29 08:47:06) >> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued >> work needs to be flushed or cancelled to ensure that the next time it starts, >> it won't process stale data. So remove all messages targeting the `Debayer` >> object on the worker thread. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/software_isp/software_isp.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index b7651b7d2..fdadf79e1 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() >> { >> ispWorkerThread_.exit(); >> ispWorkerThread_.wait(); >> + ispWorkerThread_.removeMessages(debayer_.get()); > > As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ > messages be dropped? I believe there is only the debayer object that lives on that thread and receives messages. > > Should we wrap a helper to the thread class that will > - Exit the thread > - Block until it's stopped > - Remove all messages ? > > That seems like a common 'requirement' for a thread that could be > restarted perhaps ? I'm not sure. I would think it depends on the exact setup and the objects in question. And also one usually has to do two things: clear/flush the messages on the thread, and then clear/flush all signals that came from that thread. And as far as I can tell, this change will be superseded by the introduction of gpu debayering because that introduces a blocking "stop()" call which will flush all messages. Regards, Barnabás Pőcze > > > >> >> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); >> >> -- >> 2.51.0 >>
Quoting Barnabás Pőcze (2025-10-02 09:11:28) > Hi > > 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-09-29 08:47:06) > >> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued > >> work needs to be flushed or cancelled to ensure that the next time it starts, > >> it won't process stale data. So remove all messages targeting the `Debayer` > >> object on the worker thread. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/libcamera/software_isp/software_isp.cpp | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >> index b7651b7d2..fdadf79e1 100644 > >> --- a/src/libcamera/software_isp/software_isp.cpp > >> +++ b/src/libcamera/software_isp/software_isp.cpp > >> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() > >> { > >> ispWorkerThread_.exit(); > >> ispWorkerThread_.wait(); > >> + ispWorkerThread_.removeMessages(debayer_.get()); > > > > As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ > > messages be dropped? > > I believe there is only the debayer object that lives on that thread and > receives messages. > > > > > > Should we wrap a helper to the thread class that will > > - Exit the thread > > - Block until it's stopped > > - Remove all messages ? > > > > That seems like a common 'requirement' for a thread that could be > > restarted perhaps ? > > I'm not sure. I would think it depends on the exact setup and the objects > in question. And also one usually has to do two things: clear/flush the > messages on the thread, and then clear/flush all signals that came from > that thread. > > And as far as I can tell, this change will be superseded by the introduction of > gpu debayering because that introduces a blocking "stop()" call which will flush > all messages. I see. Do you still want to merge this patch ? If so I'm fine with it. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Regards, > Barnabás Pőcze > > > > > > > > >> > >> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); > >> > >> -- > >> 2.51.0 > >> >
2025. 10. 02. 11:28 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-10-02 09:11:28) >> Hi >> >> 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-09-29 08:47:06) >>>> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued >>>> work needs to be flushed or cancelled to ensure that the next time it starts, >>>> it won't process stale data. So remove all messages targeting the `Debayer` >>>> object on the worker thread. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/libcamera/software_isp/software_isp.cpp | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>>> index b7651b7d2..fdadf79e1 100644 >>>> --- a/src/libcamera/software_isp/software_isp.cpp >>>> +++ b/src/libcamera/software_isp/software_isp.cpp >>>> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() >>>> { >>>> ispWorkerThread_.exit(); >>>> ispWorkerThread_.wait(); >>>> + ispWorkerThread_.removeMessages(debayer_.get()); >>> >>> As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ >>> messages be dropped? >> >> I believe there is only the debayer object that lives on that thread and >> receives messages. >> >> >>> >>> Should we wrap a helper to the thread class that will >>> - Exit the thread >>> - Block until it's stopped >>> - Remove all messages ? >>> >>> That seems like a common 'requirement' for a thread that could be >>> restarted perhaps ? >> >> I'm not sure. I would think it depends on the exact setup and the objects >> in question. And also one usually has to do two things: clear/flush the >> messages on the thread, and then clear/flush all signals that came from >> that thread. >> >> And as far as I can tell, this change will be superseded by the introduction of >> gpu debayering because that introduces a blocking "stop()" call which will flush >> all messages. > > > I see. Do you still want to merge this patch ? If so I'm fine with it. Yes. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> >> >> Regards, >> Barnabás Pőcze >> >>> >>> >>> >>>> >>>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); >>>> >>>> -- >>>> 2.51.0 >>>> >>
Hi all, Does anyone else have any comments or thoughts on this patch? Barnabás tells me he'd like it in the 0.6 release .... Any blockers to merging ? Quoting Barnabás Pőcze (2025-10-02 10:45:41) > 2025. 10. 02. 11:28 keltezéssel, Kieran Bingham írta: > > Quoting Barnabás Pőcze (2025-10-02 09:11:28) > >> Hi > >> > >> 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: > >>> Quoting Barnabás Pőcze (2025-09-29 08:47:06) > >>>> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued > >>>> work needs to be flushed or cancelled to ensure that the next time it starts, > >>>> it won't process stale data. So remove all messages targeting the `Debayer` > >>>> object on the worker thread. > >>>> > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>> --- > >>>> src/libcamera/software_isp/software_isp.cpp | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >>>> index b7651b7d2..fdadf79e1 100644 > >>>> --- a/src/libcamera/software_isp/software_isp.cpp > >>>> +++ b/src/libcamera/software_isp/software_isp.cpp > >>>> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() > >>>> { > >>>> ispWorkerThread_.exit(); > >>>> ispWorkerThread_.wait(); > >>>> + ispWorkerThread_.removeMessages(debayer_.get()); > >>> > >>> As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ > >>> messages be dropped? > >> > >> I believe there is only the debayer object that lives on that thread and > >> receives messages. > >> > >> > >>> > >>> Should we wrap a helper to the thread class that will > >>> - Exit the thread > >>> - Block until it's stopped > >>> - Remove all messages ? > >>> > >>> That seems like a common 'requirement' for a thread that could be > >>> restarted perhaps ? > >> > >> I'm not sure. I would think it depends on the exact setup and the objects > >> in question. And also one usually has to do two things: clear/flush the > >> messages on the thread, and then clear/flush all signals that came from > >> that thread. > >> > >> And as far as I can tell, this change will be superseded by the introduction of > >> gpu debayering because that introduces a blocking "stop()" call which will flush > >> all messages. > > > > > > I see. Do you still want to merge this patch ? If so I'm fine with it. > > Yes. > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> > >> > >> Regards, > >> Barnabás Pőcze > >> > >>> > >>> > >>> > >>>> > >>>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); > >>>> > >>>> -- > >>>> 2.51.0 > >>>> > >> >
On 10/13/25 11:53, Kieran Bingham wrote: > Hi all, > > Does anyone else have any comments or thoughts on this patch? > > Barnabás tells me he'd like it in the 0.6 release .... > > Any blockers to merging ? Quickly tested it on two devices with swISP and didn't spot any regressions. And from what I can see it would be good to have in 0.6 until a blocking stop call gets introduced, so thumbsup from my side. > Quoting Barnabás Pőcze (2025-10-02 10:45:41) >> 2025. 10. 02. 11:28 keltezéssel, Kieran Bingham írta: >>> Quoting Barnabás Pőcze (2025-10-02 09:11:28) >>>> Hi >>>> >>>> 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2025-09-29 08:47:06) >>>>>> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued >>>>>> work needs to be flushed or cancelled to ensure that the next time it starts, >>>>>> it won't process stale data. So remove all messages targeting the `Debayer` >>>>>> object on the worker thread. >>>>>> >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/software_isp/software_isp.cpp | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>>>>> index b7651b7d2..fdadf79e1 100644 >>>>>> --- a/src/libcamera/software_isp/software_isp.cpp >>>>>> +++ b/src/libcamera/software_isp/software_isp.cpp >>>>>> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() >>>>>> { >>>>>> ispWorkerThread_.exit(); >>>>>> ispWorkerThread_.wait(); >>>>>> + ispWorkerThread_.removeMessages(debayer_.get()); >>>>> As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ >>>>> messages be dropped? >>>> I believe there is only the debayer object that lives on that thread and >>>> receives messages. >>>> >>>> >>>>> Should we wrap a helper to the thread class that will >>>>> - Exit the thread >>>>> - Block until it's stopped >>>>> - Remove all messages ? >>>>> >>>>> That seems like a common 'requirement' for a thread that could be >>>>> restarted perhaps ? >>>> I'm not sure. I would think it depends on the exact setup and the objects >>>> in question. And also one usually has to do two things: clear/flush the >>>> messages on the thread, and then clear/flush all signals that came from >>>> that thread. >>>> >>>> And as far as I can tell, this change will be superseded by the introduction of >>>> gpu debayering because that introduces a blocking "stop()" call which will flush >>>> all messages. >>> >>> I see. Do you still want to merge this patch ? If so I'm fine with it. >> Yes. >> >> >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>>> >>>> Regards, >>>> Barnabás Pőcze >>>> >>>>> >>>>> >>>>>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); >>>>>> >>>>>> -- >>>>>> 2.51.0 >>>>>>
Quoting Robert Mader (2025-10-13 11:40:41) > On 10/13/25 11:53, Kieran Bingham wrote: > > Hi all, > > > > Does anyone else have any comments or thoughts on this patch? > > > > Barnabás tells me he'd like it in the 0.6 release .... > > > > Any blockers to merging ? > > Quickly tested it on two devices with swISP and didn't spot any regressions. Can we turn that into a Tested-by ... ? > > And from what I can see it would be good to have in 0.6 until a blocking > stop call gets introduced, so thumbsup from my side. > > > Quoting Barnabás Pőcze (2025-10-02 10:45:41) > >> 2025. 10. 02. 11:28 keltezéssel, Kieran Bingham írta: > >>> Quoting Barnabás Pőcze (2025-10-02 09:11:28) > >>>> Hi > >>>> > >>>> 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: > >>>>> Quoting Barnabás Pőcze (2025-09-29 08:47:06) > >>>>>> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued > >>>>>> work needs to be flushed or cancelled to ensure that the next time it starts, > >>>>>> it won't process stale data. So remove all messages targeting the `Debayer` > >>>>>> object on the worker thread. > >>>>>> > >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>>> --- > >>>>>> src/libcamera/software_isp/software_isp.cpp | 1 + > >>>>>> 1 file changed, 1 insertion(+) > >>>>>> > >>>>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >>>>>> index b7651b7d2..fdadf79e1 100644 > >>>>>> --- a/src/libcamera/software_isp/software_isp.cpp > >>>>>> +++ b/src/libcamera/software_isp/software_isp.cpp > >>>>>> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() > >>>>>> { > >>>>>> ispWorkerThread_.exit(); > >>>>>> ispWorkerThread_.wait(); > >>>>>> + ispWorkerThread_.removeMessages(debayer_.get()); > >>>>> As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ > >>>>> messages be dropped? > >>>> I believe there is only the debayer object that lives on that thread and > >>>> receives messages. > >>>> > >>>> > >>>>> Should we wrap a helper to the thread class that will > >>>>> - Exit the thread > >>>>> - Block until it's stopped > >>>>> - Remove all messages ? > >>>>> > >>>>> That seems like a common 'requirement' for a thread that could be > >>>>> restarted perhaps ? > >>>> I'm not sure. I would think it depends on the exact setup and the objects > >>>> in question. And also one usually has to do two things: clear/flush the > >>>> messages on the thread, and then clear/flush all signals that came from > >>>> that thread. > >>>> > >>>> And as far as I can tell, this change will be superseded by the introduction of > >>>> gpu debayering because that introduces a blocking "stop()" call which will flush > >>>> all messages. > >>> > >>> I see. Do you still want to merge this patch ? If so I'm fine with it. > >> Yes. > >> > >> > >>> > >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> > >>>> > >>>> Regards, > >>>> Barnabás Pőcze > >>>> > >>>>> > >>>>> > >>>>>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); > >>>>>> > >>>>>> -- > >>>>>> 2.51.0 > >>>>>> > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 >
On 10/13/25 12:58, Kieran Bingham wrote: > Quoting Robert Mader (2025-10-13 11:40:41) >> On 10/13/25 11:53, Kieran Bingham wrote: >>> Hi all, >>> >>> Does anyone else have any comments or thoughts on this patch? >>> >>> Barnabás tells me he'd like it in the 0.6 release .... >>> >>> Any blockers to merging ? >> Quickly tested it on two devices with swISP and didn't spot any regressions. > Can we turn that into a > > Tested-by ... ? Not super throughout, but yes: Tested-by: Robert Mader<robert.mader@collabora.com> >> And from what I can see it would be good to have in 0.6 until a blocking >> stop call gets introduced, so thumbsup from my side. >> >>> Quoting Barnabás Pőcze (2025-10-02 10:45:41) >>>> 2025. 10. 02. 11:28 keltezéssel, Kieran Bingham írta: >>>>> Quoting Barnabás Pőcze (2025-10-02 09:11:28) >>>>>> Hi >>>>>> >>>>>> 2025. 10. 01. 17:58 keltezéssel, Kieran Bingham írta: >>>>>>> Quoting Barnabás Pőcze (2025-09-29 08:47:06) >>>>>>>> Debayering is carried out on `ispWorkerThread_`. When stopping, the queued >>>>>>>> work needs to be flushed or cancelled to ensure that the next time it starts, >>>>>>>> it won't process stale data. So remove all messages targeting the `Debayer` >>>>>>>> object on the worker thread. >>>>>>>> >>>>>>>> Signed-off-by: Barnabás Pőcze<barnabas.pocze@ideasonboard.com> >>>>>>>> --- >>>>>>>> src/libcamera/software_isp/software_isp.cpp | 1 + >>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>> >>>>>>>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>>>>>>> index b7651b7d2..fdadf79e1 100644 >>>>>>>> --- a/src/libcamera/software_isp/software_isp.cpp >>>>>>>> +++ b/src/libcamera/software_isp/software_isp.cpp >>>>>>>> @@ -358,6 +358,7 @@ void SoftwareIsp::stop() >>>>>>>> { >>>>>>>> ispWorkerThread_.exit(); >>>>>>>> ispWorkerThread_.wait(); >>>>>>>> + ispWorkerThread_.removeMessages(debayer_.get()); >>>>>>> As the ispWorkerThread_ has just been asked to exit() shouldn't /all/ >>>>>>> messages be dropped? >>>>>> I believe there is only the debayer object that lives on that thread and >>>>>> receives messages. >>>>>> >>>>>> >>>>>>> Should we wrap a helper to the thread class that will >>>>>>> - Exit the thread >>>>>>> - Block until it's stopped >>>>>>> - Remove all messages ? >>>>>>> >>>>>>> That seems like a common 'requirement' for a thread that could be >>>>>>> restarted perhaps ? >>>>>> I'm not sure. I would think it depends on the exact setup and the objects >>>>>> in question. And also one usually has to do two things: clear/flush the >>>>>> messages on the thread, and then clear/flush all signals that came from >>>>>> that thread. >>>>>> >>>>>> And as far as I can tell, this change will be superseded by the introduction of >>>>>> gpu debayering because that introduces a blocking "stop()" call which will flush >>>>>> all messages. >>>>> I see. Do you still want to merge this patch ? If so I'm fine with it. >>>> Yes. >>>> >>>> >>>>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com> >>>>> >>>>>> Regards, >>>>>> Barnabás Pőcze >>>>>> >>>>>>> >>>>>>>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); >>>>>>>> >>>>>>>> -- >>>>>>>> 2.51.0 >>>>>>>> >> -- >> Robert Mader >> Consultant Software Developer >> >> Collabora Ltd. >> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK >> Registered in England & Wales, no. 5513718 >>
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index b7651b7d2..fdadf79e1 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -358,6 +358,7 @@ void SoftwareIsp::stop() { ispWorkerThread_.exit(); ispWorkerThread_.wait(); + ispWorkerThread_.removeMessages(debayer_.get()); Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
Debayering is carried out on `ispWorkerThread_`. When stopping, the queued work needs to be flushed or cancelled to ensure that the next time it starts, it won't process stale data. So remove all messages targeting the `Debayer` object on the worker thread. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/software_isp/software_isp.cpp | 1 + 1 file changed, 1 insertion(+)