Segment and pause #531

bebon901 · 2023-07-05T10:40:12Z

For review - DO NOT MERGE YET
@davidplowman @naushir @njhollinghurst

Have added the code to segment, pause, pause & split. Also updated codestyle

core/libcamera_encoder.hpp

encoder/libav_encoder.cpp

njhollinghurst · 2023-07-05T13:48:59Z

output/output.cpp

@@ -13,6 +13,8 @@
 #include "net_output.hpp"
 #include "output.hpp"

+bool enable_ = true;


How does this relate to the member variable of the same name?
I'm not even sure which one will be in scope, and I expect it will confuse other readers as well.

(Underscores on the end of a something normally imply a data member in a class)

Indeed. All these funny global variables need to go, unless there is some special justification for them.

njhollinghurst · 2023-07-05T13:55:52Z

encoder/libav_encoder.cpp

@@ -321,21 +332,70 @@ LibAvEncoder::~LibAvEncoder()
 	LOG(2, "libav: codec closed");
 }

+int64_t previous_time = 0;


Should this be a class member? (Is it different from previous_timestamp?)

njhollinghurst · 2023-07-05T13:56:20Z

encoder/libav_encoder.cpp

 	av_packet_free(&pkt);
 	deinitOutput();
 }

+
+int64_t previous_timestamp = 0;


Another mysterious static variable

njhollinghurst · 2023-07-05T14:03:06Z

encoder/encoder.hpp

@@ -33,6 +33,8 @@ class Encoder
 	// describing a DMABUF, and by a mmapped userland pointer.
 	virtual void EncodeBuffer(int fd, size_t size, void *mem, StreamInfo const &info, int64_t timestamp_us) = 0;

+	virtual void Signal() {}


Perhaps a comment could be added above to explain what this method is for.

I don't want to be too nit-picky, but all the other comments appear above their respective function declarations.

I wonder if there's some way the wording could be a little more general/abstract, so that other Encoder sub-class writers will know what to do (or why they might or might not need to implement it)?

Agree, probably just move the comments before the declaration.

davidplowman · 2023-07-05T21:14:09Z

encoder/libav_encoder.cpp

-
+		std::string filename = options_->output;			
+		char subfilename[256];
+		snprintf(subfilename, sizeof(subfilename), options_->output.c_str(), segment_num);


Is this one of those evil occasions when you can wind up without the terminating null if the given string was too long?

snprintf is safe wrt the NUL termination.

There is still a risk if output contained other unexpected conversion sequences (e.g. "%s%s%s"), which could cause it to read undefined values and crash. But I don't think this application is critical enough to bother checking this!

davidplowman · 2023-07-05T21:15:31Z

encoder/libav_encoder.cpp

+		char subfilename[256];
+		snprintf(subfilename, sizeof(subfilename), options_->output.c_str(), segment_num);
+		filename = subfilename;
+		printf("Outputting with filename: %s\n", filename.c_str());


Just wanted to check whether printf is the right thing here, or whether it should be some kind of logging statement?

davidplowman · 2023-07-05T21:16:36Z

encoder/libav_encoder.cpp

@@ -630,6 +704,8 @@ void LibAvEncoder::audioThread()
 		av_frame_unref(in_frame);
 		av_packet_unref(in_pkt);

+		std::this_thread::yield(); // Added to try to reduce thread load on proccessor
+


Just a bit intrigued why this is necessary... do we know anything more?

@naushir requested this to be added
Seems to put the thread to sleep whilst we wait for a frame as oppose to constantly looping

You can remove this for now - I'm not 100% sure if it's needed and we probably want to do a bit more experimenting.

davidplowman · 2023-07-05T21:19:29Z

Hi Ben, mostly this looks pretty good to me with the suggested tidy ups. @njhollinghurst Maybe ask Nick to show you how to squash some of those commits together that are patching up style problems, just to leave us with a trail that's easier to follow?

naushir · 2023-07-06T08:58:32Z

Just a quick check @bebon901 - did you manage to do a test run with audio encode enabled?

naushir · 2023-07-06T08:59:04Z

PS sorry I have not had a chance to review yet, I will do when we get back to the office tomorrow!

bebon901 · 2023-07-06T09:04:14Z

Just a quick check @bebon901 - did you manage to do a test run with audio encode enabled?
@naushir, Did manage, was all working. Will check once more before I rePush

bebon901 · 2023-07-06T15:50:29Z

I've changed the things you wanted, please can you review again?

Added segmentation, pause and split to LibAV encoding when running libcamera-vid. Tidied up and added comments on various files used. Signed-off-by: Ben Benson <[email protected]>

davidplowman · 2023-07-07T12:29:09Z

encoder/libav_encoder.cpp

@@ -17,7 +17,8 @@
 #include <iostream>

 #include "libav_encoder.hpp"
-
+int64_t segment_start_ts = 0;
+int segment_num = 0; 


Do these really want to be data members in the class?

davidplowman · 2023-07-07T12:29:33Z

encoder/libav_encoder.cpp

@@ -323,12 +324,28 @@ LibAvEncoder::~LibAvEncoder()

 void LibAvEncoder::EncodeBuffer(int fd, size_t size, void *mem, StreamInfo const &info, int64_t timestamp_us)
 {
+


Can probably delete the extra blank line...

davidplowman · 2023-07-07T12:31:29Z

encoder/libav_encoder.cpp

 	AVFrame *frame = av_frame_alloc();
 	if (!frame)
 		throw std::runtime_error("libav: could not allocate AVFrame");

-	if (!video_start_ts_)
+	if (!video_start_ts_) {


In libcamera-apps we tend to add a line break before a curly brace.

davidplowman · 2023-07-07T12:32:06Z

encoder/libav_encoder.cpp

 		video_start_ts_ = timestamp_us;
+		segment_start_ts = video_start_ts_;
+	}
+	printf("Frame added \n");


We should probably turn these into LOG statements?

davidplowman · 2023-07-07T12:40:13Z

encoder/libav_encoder.cpp

-
+		if (options_->segment){			
+			char subfilename[256];
+			snprintf(subfilename, sizeof(subfilename), options_->output.c_str(), segment_num);


I have a bad feeling this might crash if options_->output.c_str() is too long?

Ah no, re-reading the snprintf documentation this does indeed seem to be oK!!

davidplowman · 2023-07-07T12:40:44Z

encoder/libav_encoder.cpp

+	printf("Frame added \n");
+	if ((options_->segment && (timestamp_us - segment_start_ts)/ 1000 > options_->segment) && output_ready_)
+		{	
+			printf("Segment Added \n");


Same comment as above applies to all printfs (I shan't repeat it elsewhere!).

davidplowman · 2023-07-07T12:41:49Z

encoder/libav_encoder.cpp

@@ -411,13 +434,15 @@ void LibAvEncoder::initOutput()

 void LibAvEncoder::deinitOutput()
 {
+


Remove the blank line? (It's not that the blank line matters, it's just that diffs are easier to look at the smaller they are!!)

davidplowman · 2023-07-07T12:42:15Z

encoder/libav_encoder.cpp

@@ -444,6 +469,7 @@ void LibAvEncoder::encode(AVPacket *pkt, unsigned int stream_id)
 			output_ready_ = true;
 		}

+


Blank line?

davidplowman · 2023-07-07T12:42:24Z

encoder/libav_encoder.cpp

@@ -487,9 +513,9 @@ void LibAvEncoder::videoThread()
 				using namespace std::chrono_literals;
 				// Must check the abort first, to allow items in the output
 				// queue to have a callback.
-				if (abort_video_ && frame_queue_.empty())
+				if (abort_video_ && frame_queue_.empty()){


Curly brace?

encoder/libav_encoder.cpp

davidplowman · 2023-07-07T12:43:42Z

encoder/libav_encoder.cpp

@@ -658,11 +688,11 @@ void LibAvEncoder::audioThread()

 			out_frame->pts = ts + (options_->av_sync > 0 ? options_->av_sync : 0);
 			audio_samples_ += codec_ctx_[AudioOut]->frame_size;
-
+			std::scoped_lock<std::mutex> lock(reset_mutex_); {


Here too, let's just check if the lock should be inside the curly brace.

encoder/libav_encoder.cpp

davidplowman · 2023-07-07T14:03:03Z

encoder/libav_encoder.cpp

 	{ "h264_v4l2m2m", encoderOptionsH264M2M },
 	{ "libx264", encoderOptionsLibx264 },
 };

 } // namespace

+void LibAvEncoder::Signal()
+{
+	feed_encoder_frames = !feed_encoder_frames;


I wonder if feed_encoder_frames should strictly be feed_encoder_frames_, just to maintain the convention about data member naming.

davidplowman · 2023-07-07T14:03:38Z

encoder/libav_encoder.cpp

 		video_start_ts_ = timestamp_us;
+		segment_start_ts = video_start_ts_;


segment_start_ts_?

davidplowman · 2023-07-07T14:08:34Z

encoder/libav_encoder.hpp

+	int64_t previous_video_timestamp;
+	int64_t previous_audio_timestamp;
+	bool feed_encoder_frames;
+	bool previous_feed_value;


All fine, just want the trailing underscore at the end of each name.

naushir · 2023-07-07T14:02:26Z

encoder/libav_encoder.cpp

-namespace {
+
+namespace
+{


Extra whitespace? (Sorry...)

naushir · 2023-07-07T14:03:00Z

encoder/libav_encoder.cpp

@@ -54,14 +58,18 @@ void encoderOptionsLibx264(VideoOptions const *options, AVCodecContext *codec)
 	av_opt_set(codec->priv_data, "mixed_ref", "0", 0);
 }

-const std::map<std::string, std::function<void(VideoOptions const *, AVCodecContext *)>> optionsMap =
-{
+const std::map<std::string, std::function<void(VideoOptions const *, AVCodecContext *)>> optionsMap = {


Can we revert this format change?

naushir · 2023-07-07T14:03:40Z

encoder/libav_encoder.cpp

 void LibAvEncoder::EncodeBuffer(int fd, size_t size, void *mem, StreamInfo const &info, int64_t timestamp_us)
 {
 	AVFrame *frame = av_frame_alloc();
+


Extra line can be removed.

naushir · 2023-07-07T14:05:03Z

encoder/libav_encoder.hpp

+	int64_t previous_video_timestamp;
+	int64_t previous_audio_timestamp;
+	bool feed_encoder_frames;
+	bool previous_feed_value;


All these want to have a _ suffix to denote they are class members.

naushir · 2023-07-07T14:05:17Z

encoder/libav_encoder.hpp

 	std::condition_variable video_cv_;
 	std::thread video_thread_;
 	std::thread audio_thread_;

 	// The ordering in the enum below must not change!
-	enum Context { Video = 0, AudioOut = 1, AudioIn = 2 };
+	enum Context


Can we revert this format change?

naushir · 2023-07-07T14:13:58Z

encoder/libav_encoder.cpp

+			int64_t et = ts - previous_audio_timestamp; // Elapsed Time
+
+			if (feed_encoder_frames && options_->keypress)
+			{ // If feeding audio and keypress option is on, then count the frame timer for the audio frames


Comment on the next line.

naushir · 2023-07-07T14:14:15Z

encoder/libav_encoder.cpp

+			}
+			else
+			{
+			out_frame->pts = ts - virtual_audio_ts + (options_->av_sync < 0 ? -options_->av_sync : 0);


Indent, and remove brackets?

naushir · 2023-07-07T14:14:25Z

encoder/libav_encoder.cpp

+				{ // If recording, or not.
+					ret = avcodec_send_frame(codec_ctx_[AudioOut], out_frame);
+					if (ret < 0)
+					{


Remove brackets?

naushir · 2023-07-07T14:14:34Z

encoder/libav_encoder.cpp

+			std::scoped_lock<std::mutex> lock(reset_mutex_);
+			{
+				if (feed_encoder_frames)
+				{ // If recording, or not.


Comment on the next line.

naushir · 2023-07-07T14:14:49Z

encoder/libav_encoder.cpp

+			}
+			previous_audio_timestamp = ts; // Previous TimeStamp
+			audio_samples_ += codec_ctx_[AudioOut]->frame_size;
+			std::scoped_lock<std::mutex> lock(reset_mutex_);


Scoped lock inside the bracket below?

davidplowman · 2023-07-07T14:16:07Z

encoder/libav_encoder.cpp

 	av_packet_free(&pkt);
 	deinitOutput();
 }

+


Blank line?

davidplowman · 2023-07-07T14:17:02Z

encoder/libav_encoder.cpp

+			}
+			previous_audio_timestamp = ts; // Previous TimeStamp
+			audio_samples_ += codec_ctx_[AudioOut]->frame_size;
+			std::scoped_lock<std::mutex> lock(reset_mutex_);


Maybe just check if this lock want to be inside or outside the braces?

bebon901 · 2023-07-10T09:05:56Z

Have now resolved all previous issues.

davidplowman · 2023-07-10T09:09:52Z

Hi Ben, this is looking pretty good to me now. The only final thing I would suggest would be to squash both commits together. The 2nd is just style/cosmetic fixes, so by making it look as though you got this all perfect first time round then we actually end up with fewer changes for future generations to scratch their heads over...!

davidplowman · 2023-07-11T12:50:15Z

encoder/libav_encoder.hpp

+	int64_t previous_video_timestamp;
+	int64_t previous_audio_timestamp;
+	bool feed_encoder_frames;
+	bool previous_feed_value;


It's looking to me a bit like we have versions with underscores and without. Have I read that right? If so, the underscoreless versions want to go!

davidplowman · 2023-07-11T12:56:40Z

encoder/libav_encoder.cpp

+			previous_audio_timestamp_ = ts; // Previous TimeStamp
+			audio_samples_ += codec_ctx_[AudioOut]->frame_size;
+			{
+				std::scoped_lock<std::mutex> lock(reset_mutex_);


Just wondering about the placement of the lock. In the video thread, the lock covers the call to encode() but here it doesn't. Is one of them incorrect maybe?

I think it ought to ~~not~~ cover encode() in both cases. But we ought to test this thoroughly.

Have removed mutexes. Seems to record very long videos with lots of segments without issue. Have confirmed audio also works.

davidplowman · 2023-07-11T13:42:49Z

encoder/libav_encoder.cpp

+		nextSegment(timestamp_us, segment_start_ts_);
+	// Split Function
+	//want to catch the 'rising edge' of the feed_frames variable. when it rises, we want to save the file and start with a new one
+	if (options_->split && (previous_feed_value_ != feed_encoder_frames_) && feed_encoder_frames_)


We wouldn't normally bother with the brackets round the "not equals" comparison, but I'm happy to ignore.

encoder/libav_encoder.hpp

naushir · 2023-07-11T13:47:04Z

encoder/libav_encoder.cpp

@@ -388,6 +423,15 @@ void LibAvEncoder::initOutput()
 	if (!(out_fmt_ctx_->flags & AVFMT_NOFILE))
 	{
 		std::string filename = options_->output;
+		char subfilename[256];
+		int n;
+		n = snprintf(subfilename, sizeof(subfilename), options_->output.c_str(), segment_num_);


Shouldn't this be conditional on setting the segment option?

encoder/libav_encoder.cpp

davidplowman · 2023-07-11T13:49:05Z

Looks good to me. Happy to merge if Naush is.

At the risk of one final nit-pick, in the commit message, "Signed-off-by" should be "Signed-off-by:". Sorry!!

naushir · 2023-07-11T13:51:16Z

Looks good to me. Happy to merge if Naush is.

Just some minor clarifications/fixes for the encode() call and filename handling, otherwise it looks good!

naushir · 2023-07-11T13:52:14Z

encoder/libav_encoder.cpp

+					if (ret < 0)
+						throw std::runtime_error("libav: error encoding frame: " + std::to_string(ret));
+				}
+			}
 			encode(out_pkt, AudioOut);


Also here should encode be called only when feed_encoder_frames_ is true?

encoder/libav_encoder.cpp

bebon901 · 2023-07-11T14:05:08Z

All above issues have been resolved. Have tested with audio, without audio, segmented, not segmented and paused/not paused.

bebon901 closed this Jul 5, 2023

bebon901 reopened this Jul 5, 2023

njhollinghurst reviewed Jul 5, 2023

View reviewed changes

core/libcamera_encoder.hpp Show resolved Hide resolved

njhollinghurst reviewed Jul 5, 2023

View reviewed changes

encoder/libav_encoder.cpp Outdated Show resolved Hide resolved

njhollinghurst reviewed Jul 5, 2023

View reviewed changes

davidplowman reviewed Jul 5, 2023

View reviewed changes

bebon901 force-pushed the SegmentandPause branch from ccd9adf to 1018544 Compare July 6, 2023 09:59

encoder: LibAV Segmentation and Pause Functions

801a6fa

Added segmentation, pause and split to LibAV encoding when running libcamera-vid. Tidied up and added comments on various files used. Signed-off-by: Ben Benson <[email protected]>