Multiple encoder support #588

chrisruk · 2023-02-24T16:20:48Z

No description provided.

chrisruk · 2023-03-01T15:16:16Z

picamera2/picamera2.py

@@ -873,8 +873,6 @@ def configure_(self, camera_config="preview") -> None:
        """
        if self.started:
            raise RuntimeError("Camera must be stopped before configuring")
-        if self.encoder is not None and self.encoder.running:


Removed this check, to allow starting multiple encoders

It feels to me as though we still want something like this. It would need to complain if any encoder in the set is still running. Does that sound correct?

chrisruk · 2023-03-01T15:17:55Z

picamera2/picamera2.py

@@ -937,9 +935,6 @@ def configure_(self, camera_config="preview") -> None:
        self.encode_stream_name = camera_config['encode']
        if self.encode_stream_name is not None and self.encode_stream_name not in camera_config:
            raise RuntimeError(f"Encode stream {self.encode_stream_name} was not defined")
-        elif self.encode_stream_name is None:


Removed this check as encoders don't need to always encode from encode_stream_name

davidplowman · 2023-03-06T09:36:50Z

Hi Chris, what's the status on this one, should I be trying this PR out now?

chrisruk · 2023-03-06T09:49:29Z

Hi David, yeah if you could try it out, that'd be great.

davidplowman · 2023-03-06T12:06:05Z

Hi Chris, I noticed a couple of things and also there are just a couple of things I want to think about a bit more.

Firstly, I noticed that when I ran the example, the resolution of the MJPEG was the same as the h.264 file! The problem is this line where request.picam2.encode_stream_name needs to become self.name.

Fixing this actually breaks the example because the s/w JpegEncoder can't accept YUV420, but it does work if you use the MJPEGEncoder instead which is fine (at least, it's how things are).

The next thing I tried was a modified version of the example that went like this:

picam2.start_recording(encoder1, 'test1.h264')
time.sleep(5)
picam2.start_encoder(encoder2, 'test.mjpeg', name="lores")
time.sleep(5)
picam2.stop_encoder(encoder2)
time.sleep(5)
picam2.stop_recording()

The point of the example being to start and stop one encoder while another is running continuously.

The first little issue I had was when starting encoder2. I found that in start_encoder I had to add the encoder to the list (actually set) of encoders at the very end, after the encoder is started. Otherwise process_requests(), running in the event loop, may pick up the encoder before it's all configured, and fall over.

This brings me to the final question - do we think that stop_encoder should be able to stop just one encoder? At the moment it stops all of them, which makes this particular use case more awkward. Perhaps we should have a new stop_encoders that stops all of them? And ditto for stop_recording? I'm not too sure about this as it's all starting to feel a bit fussy, I wonder if there's any way to make it seem tidier. What do you think?

chrisruk · 2023-03-06T14:15:39Z

Thanks a lot for the feedback. I hadn't noticed that about request.picam2.encode_stream_name in the multiencoder, I just noticed request.picam2.encode_stream_name is also present in the encoder class, will get that sorted soon.

Could picam2.stop_encoder stop all encoders if no parameter is passed, and if you pass an encoder it then just stops that one maybe? And for stop_recording, it could do the same, but stop the camera beforehand?

davidplowman · 2023-03-06T14:54:50Z

Yes, maybe stop_encoder takes an encoders parameter which it interprets as

None (which would be the default) - stop all of them
A single encoder - just stop that one
A list - stop the encoders in the list.

That feels OK, I think.

As regards stop_recording, maybe that just stops all of them. It stops the camera too, so presumably you're kind of done, and you can use stop_encoder if that's not what you wanted.

One other thing I wondered, whether we should remove an encoder from the list (or set) when we stop it (seeing as we insert it when we start, so it would satisfy my need for symmetry). There is actually a race condition between removing it from the set and whether process_requests still sees the encoder, but I think we've already ensured that sending another frame to an encoder that's stopped is safe, so that's probably OK.

chrisruk · 2023-03-08T12:04:17Z

Realised need to work on case when start_encoder is called with encoder = None, which I'll start on soon.

davidplowman · 2023-03-08T12:04:58Z

No prob, give me a shout when you want me to try it again!

chrisruk · 2023-03-09T13:31:36Z

@davidplowman Just wondering if you'd be able to take another look. Thanks!

I modified the start_encoder method, so that it is able to start an encoder automatically, if there is 1 in the set. If there are multiple it throws an exception and asks you to choose one.

I found it was necessary when modifying the set to take ownership of the lock, so that there wasn't an exception thrown in process_requests.

I've also modified / tested some of the examples which aren't in the test list such as capture_circular_stream.py, to work with the modifications made in earlier commits.

davidplowman · 2023-03-09T14:05:01Z

Yes, seems to be working really nicely now. Let me just do the usual look over code changes and then we'll hopefully be all good!

davidplowman · 2023-03-09T14:10:53Z

picamera2/picamera2.py

@@ -270,7 +270,7 @@ def _reset_flags(self) -> None:
        self.frames = 0
        self._job_list = []
        self.options = {}
-        self._encoder = None
+        self._encoder = set()


I wonder about renaming _encoder to _encoders, and the encoder property to encoders. It might feel a bit more natural, both for users and anyone reading the code?

Good point, will change that. Just wondering with the encoders property, should that let you assign a set of encoders, that overwrites the previous set of encoders now?

Not sure! Currently you have to use start_encoder, but that also sets up a whole load of stuff like the size etc. What would you do with an encoder that hadn't been set up "properly"? Perhaps it should only be for checking what encoders it thinks are running?

I've just made it so that you can assign a single encoder, or a set of additional encoders via the property, as a couple of examples used that method.

picamera2/picamera2.py

davidplowman · 2023-03-09T14:20:45Z

examples/capture_video_multiple.py

+encoder2 = MJPEGEncoder(10000000)
+
+picam2.start_recording(encoder1, 'test1.h264')
+picam2.start_recording(encoder2, 'test2.mjpeg', name="lores")


Ah, this was why you made it OK to start the Picamera2 object twice! :)

davidplowman · 2023-03-09T14:23:25Z

examples/capture_circular_stream.py

@@ -40,7 +40,7 @@ def server():
            stream = conn.makefile("wb")
            filestream = FileOutput(stream)
            filestream.start()
-            picam2.encoder.output = [circ, filestream]
+            encoder.output = [circ, filestream]


I suppose if we rename picam2.encoder to picam2.encoders then we could redefine picam2.encoder to give you the encoder object if there's exactly one, and otherwise complain. But maybe that's bending over backwards too much for backwards compatibility, it certainly feels like it's starting to get a bit fussy!

tests/test_list.txt

Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

Because '.encoder' of a Picamera2 object now returns a set, some examples needed to be updated. Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

chrisruk · 2023-03-10T15:57:40Z

I noticed that it seemed to be setting .framerate of the encoder, which wasn't used anywhere, so I just changed it to set _framerate, which does seem to be used.

davidplowman · 2023-03-13T14:35:52Z

Hi Chris, actually I have started getting an error as follows

Traceback (most recent call last):
  File "/home/pi/test.py", line 19, in <module>
    picam2.start_encoder(encoder2, 'test2.mjpeg', name="lores")
  File "/home/pi/picamera2/picamera2/picamera2.py", line 1470, in start_encoder
    _encoder._setup(quality)
  File "/home/pi/picamera2/picamera2/encoders/mjpeg_encoder.py", line 31, in _setup
    actual_complexity = self.width * self.height * self.framerate
AttributeError: 'MJPEGEncoder' object has no attribute 'framerate'

(Not quite sure why the tests haven't picked that up.)

Anyway, I'm thinking that I'd quite like to leave framerate as it is rather than rename it to _framerate because I can imagine someone making an encoder object and setting the framerate explicitly to some value that they want to use (just as they might also set the bitrate explicitly before calling start_encoder). What do you think?

Other than that, this is looking pretty good and I'd like to get it merged. Thanks!

chrisruk · 2023-03-13T14:46:28Z

Hi David,

Do you mind pasting your /home/pi/test.py file?

I think the reason I made it use _framerate, was it's present as that in here -
https://github.com/raspberrypi/picamera2/blob/next/picamera2/encoders/v4l2_encoder.py

I could rename that to framerate?

davidplowman · 2023-03-13T14:49:24Z

Sure, here it is:

import time

from picamera2 import Picamera2
from picamera2.encoders import H264Encoder, MJPEGEncoder

picam2 = Picamera2()
video_config = picam2.create_video_configuration(main={"size": (1280, 720), "format": "RGB888"},
                                                 lores={"size": (640, 360), "format": "YUV420"})

picam2.configure(video_config)

encoder1 = H264Encoder(10000000)
encoder2 = MJPEGEncoder()

picam2.start_recording(encoder1, 'test1.h264')
time.sleep(5)
print("Start 2nd encoder")
picam2.start_encoder(encoder2, 'test2.mjpeg', name="lores")
time.sleep(5)
picam2.stop_encoder(encoder2)
print("Stopped 2nd encoder")
time.sleep(5)
picam2.stop_recording()

I guess we just need to review our use of framerate/_framerate to be sure we've got the behaviour right everywhere!

It looks like we had a mix of framerate and _framerate, changed to former Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

chrisruk · 2023-03-13T15:24:44Z

Thanks for that test case, I've got that to pass now. I renamed _framerate to framerate in the v4l2_encoder and h264_encoder files, to make things consistent.

davidplowman · 2023-03-13T16:05:01Z

Are you happy to merge this one now?

chrisruk · 2023-03-13T16:25:31Z

Sure, am happy for this to be merged now

chrisruk force-pushed the multipleencoders branch from cb28f84 to 4cc32ea Compare March 1, 2023 15:15

chrisruk marked this pull request as ready for review March 1, 2023 15:15

chrisruk commented Mar 1, 2023

View reviewed changes

chrisruk force-pushed the multipleencoders branch from 4cc32ea to f547815 Compare March 8, 2023 11:40

chrisruk force-pushed the multipleencoders branch from f547815 to 63236ff Compare March 8, 2023 14:28

davidplowman reviewed Mar 9, 2023

View reviewed changes

picamera2/picamera2.py Outdated Show resolved Hide resolved

davidplowman reviewed Mar 9, 2023

View reviewed changes

tests/test_list.txt Show resolved Hide resolved

chrisruk added 4 commits March 9, 2023 16:24

Initial support for multiple encoders

46a2e5f

Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

Initial example for multiple encoders

8850fe3

Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

Second multiple encoder example

6536b98

Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

Update existing examples to work with new encoder syntax

86adc85

Because '.encoder' of a Picamera2 object now returns a set, some examples needed to be updated. Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

chrisruk force-pushed the multipleencoders branch from 63236ff to e6baf98 Compare March 10, 2023 14:39

Set framerate

b94dc7c

It looks like we had a mix of framerate and _framerate, changed to former Signed-off-by: Chris Richardson <christopher.richardson@raspberrypi.org>

chrisruk force-pushed the multipleencoders branch from e6baf98 to b94dc7c Compare March 13, 2023 15:08

davidplowman merged commit 3d575e3 into raspberrypi:next Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple encoder support #588

Multiple encoder support #588

chrisruk commented Feb 24, 2023

chrisruk Mar 1, 2023

davidplowman Mar 9, 2023

chrisruk Mar 1, 2023

davidplowman commented Mar 6, 2023

chrisruk commented Mar 6, 2023

davidplowman commented Mar 6, 2023

chrisruk commented Mar 6, 2023 •

edited

Loading

davidplowman commented Mar 6, 2023

chrisruk commented Mar 8, 2023

davidplowman commented Mar 8, 2023

chrisruk commented Mar 9, 2023 •

edited

Loading

davidplowman commented Mar 9, 2023

davidplowman Mar 9, 2023

chrisruk Mar 9, 2023

davidplowman Mar 9, 2023

chrisruk Mar 10, 2023

davidplowman Mar 9, 2023

davidplowman Mar 9, 2023

chrisruk commented Mar 10, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

Multiple encoder support #588

Multiple encoder support #588

Conversation

chrisruk commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidplowman commented Mar 6, 2023

chrisruk commented Mar 6, 2023

davidplowman commented Mar 6, 2023

chrisruk commented Mar 6, 2023 • edited Loading

davidplowman commented Mar 6, 2023

chrisruk commented Mar 8, 2023

davidplowman commented Mar 8, 2023

chrisruk commented Mar 9, 2023 • edited Loading

davidplowman commented Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisruk commented Mar 10, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

davidplowman commented Mar 13, 2023

chrisruk commented Mar 13, 2023

chrisruk commented Mar 6, 2023 •

edited

Loading

chrisruk commented Mar 9, 2023 •

edited

Loading