-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup avformat-based preprocessors #2665
Conversation
Thanks for the contribution @lu-zero! I definitely see value in the more compact approach you're suggesting, since we had indeed many similar sections that could be streamlined. I only have one doubt related to the changes you made on AVPacket management, probably related to my limited knowledge of its memory management internals: I see the packet is allocated once before the loop is started, and then at each loop there's a |
There is no need to allocate an encoder to mux. Makes possible to use the postprocessor without having a vp8/vp9 encoder in avcodec.
void av_packet_unref(AVPacket *pkt)
{
av_packet_free_side_data(pkt); // NO-OP we do not have side data
av_buffer_unref(&pkt->buf); // NO-OP we do not use the buffer
av_init_packet(pkt); // same as the deprecated function
pkt->data = NULL; // basic sanitization
pkt->size = 0;
} |
Ack, thanks for the clarification 👍 |
@lminiero do you have test samples I can use to make sure everything works after the changes? I have samples for opus and vp8 but not for the others. |
@lu-zero I don't have any recording in the other formats, but they should be quite easy to generate. If you have a Janus instance you control somewhere, you can open the echotest demo with a custom codec preference, e.g.:
to force a specific video codec to be used (
This will create a different mjr file for each medium (in EchoTest audio, video and data). Sending |
@lu-zero please let me know when you think the refactoring is done, so that I know when I can do a review 👍 |
Do you have a known use for the silence packets in opus? I just commented it out since locally it seems to cause more problems that it solves. If you are happy with what's in there I'd say it could land. |
Actually yes, that's quite important. When we detect a packet loss, we have to inject a silence packet, otherwise the duration of the processed stream will be broken. That's because, at least from my understanding of libogg, packets can only be added to the format with monotonically increasing packet IDs, and so skipping missing packets reduces the duration of the stream (as the 20ms that were missed are not reflected in the audio recording). That might not be important in case of very few losses, but if there's many of them, this could lead to problems, like out-of-sync audio and video streams if they need to be combined ex-post. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a look at the code, and it looks like a really useful refactoring, thanks! I just added a few minor editorial comments inline (mostly related to the project code style), and a note on the Opus silence ingestion we discussed a few days ago. I haven't tested this yet, but plan to do it soon.
@@ -585,6 +585,8 @@ janus_pp_rec_SOURCES = \ | |||
postprocessing/pp-h264.h \ | |||
postprocessing/pp-av1.c \ | |||
postprocessing/pp-av1.h \ | |||
postprocessing/pp-avformat.h \ | |||
postprocessing/pp-avformat.c \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit, can you invert the order of .c and .h file here to match the others in the list?
postprocessing/pp-av1.c
Outdated
#else | ||
vStream = avformat_new_stream(fctx, 0); | ||
|
||
vStream = janus_pp_new_video_avstream(fctx, AV_CODEC_ID_AV1, max_width, max_height);; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit: there's a double semicolon at the end.
postprocessing/pp-avformat.c
Outdated
|
||
/* We save the metadata part as a comment (see #1189) */ | ||
if(metadata) | ||
av_dict_set(&ctx->metadata, "comment", metadata, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken indentation in the above lines: per the project code style, we use tabs, not spaces.
JANUS_LOG(LOG_ERR, "Error guessing format\n"); | ||
avformat_free_context(ctx); | ||
return NULL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same indentation issues for the above lines.
|
||
/* WebM output */ | ||
fctx = janus_pp_create_avformatcontext("ogg", metadata, destination); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that if we wanted to optionally allow people to output the processing to .mka
, as you said in #2658, we'd only need to make the "ogg" part a configurable string where we can put a different format target, correct? Same for videos, I guess, where VP8/VP9 could be outputted to .mkv
rather than .webm
as we do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the plan, ideally should be easy to select the output format (mp4 or mkv or webm).
I'll try to make the postprocessor able to mux both audio and video in a single pass later probably. One step at time first though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to make the postprocessor able to mux both audio and video in a single pass later probably. One step at time first though :)
That's actually not needed, since our .mjr
files only store one media stream anyway (audio, video or data). Any muxing after that should be done separately, e.g., using ffmpeg itself, and possibly using timing info from the .mjr
header to sort out sync issues.
postprocessing/pp-opus.c
Outdated
av_packet_unref(pkt); | ||
pkt->stream_index = 0; | ||
pkt->data = buffer; | ||
pkt->size = bytes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken indentation (spaces).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like in one of the machines I tested I do not have editorconfig support. Do you happen to have a clang-format setup I can use?
pkt->stream_index = 0; | ||
pkt->data = buffer; | ||
pkt->size = bytes; | ||
pkt->pts = pkt->dts = av_rescale_q(tmp->ts - list->ts, timebase, fctx->streams[0]->time_base); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this maybe why you were getting issues with the silence packets being inserted manually as we did when using libogg directly? Does the ogg format target in ffmpeg automatically insert silence audio packets looking at what the pts is, or is still increased monotonically even in case of gaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample I used to test has the audio lasting much longer than the video, I thought the problem was in the audio, but probably it is the video having problems.
I still need to get a local setup to get better samples.
Inserting the silence seems fine with both ogg and mkv output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting the silence seems fine with both ogg and mkv output.
Ack, thanks!
I still need to get a local setup to get better samples.
There are a couple of old .mjr files you can play with in this folder of the repo: they use the old format, but should be ok if you want to test audio and video streams that have the same length.
postprocessing/pp-webm.c
Outdated
#else | ||
vStream->codec->codec_id = AV_CODEC_ID_VP8; | ||
codec_id = AV_CODEC_ID_VP8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two codec_id
lines don't need to be indented (the one below isn't, for instance), as macro ifs
for us are not like "regular" ifs.
The formatting problems should be addressed now, I hope :) |
'O', 'p', 'u', 's', 'H', 'e', 'a', 'd', | ||
1, 2, 56, 1, 128, 187, | ||
0, 0, 0, 0, 0, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops looks like I missed this space-indentation in the previous review, sorry...
Thanks! I just found one I missed before (I added a comment above), but the code changes do look fine to me. I plan to make some tests either later today or tomorrow morning, and in case they work fine for me it will be good to merge. Please make sure you sign our CLA before we get to that, though, as we won't be able to merge otherwise. |
I think I already signed the CLA, it does not show on your side? |
Mh no, it's not there. Did you fill the form at the end of the page? Maybe you only did the Github authentication on top? (the page is indeed a bit confusing in that regard) |
I completed the form at least now. |
Thanks, I do see it now! ✌️ |
av_init_packet(&avpacket); | ||
avpacket.data = (uint8_t *)buffer; | ||
avpacket.size = bytes; | ||
AVPacket *avpacket = av_packet_alloc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lu-zero any reason why this allocation is inside the loop? I see for all other codecs you just do it once before the while
, and just unref/initialize the packet at each loop, while for G.722 this is happening in the while
instead. Is it because we're actually decoding something here, rather than just saving to a container?
Yes, it felt simpler that way.
…On Tue, May 25, 2021, 11:57 Lorenzo Miniero ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In postprocessing/pp-g722.c
<#2665 (comment)>
:
> @@ -192,18 +170,17 @@ int janus_pp_g722_process(FILE *file, janus_pp_frame_packet *list, int *working)
JANUS_LOG(LOG_VERB, "Writing %d bytes out of %d (seq=%"SCNu16", step=%"SCNu16", ts=%"SCNu64", time=%"SCNu64"s)\n",
bytes, tmp->len, tmp->seq, diff, tmp->ts, (tmp->ts-list->ts)/8000);
/* Decode and save to wav */
- AVPacket avpacket;
- av_init_packet(&avpacket);
- avpacket.data = (uint8_t *)buffer;
- avpacket.size = bytes;
+ AVPacket *avpacket = av_packet_alloc();
@lu-zero <https://github.com/lu-zero> any reason why this allocation is
inside the loop? I see for all other codecs you just do it once before the
while, and just unref/initialize the packet at each loop, while for G.722
this is happening in the while instead. Is it because we're actually
decoding something here, rather than just saving to a container?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2665 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2LJHTM4Z2K3KGI6SIZ7DTPNYBZANCNFSM4454KPIQ>
.
|
if(pos >= nextPos) { | ||
JANUS_LOG(LOG_WARN, "[SKIP] pos: %06" SCNu64 ", skipping remaining silence\n", pos); | ||
JANUS_LOG(LOG_WARN, "[SKIP] pos: %06" SCNu64 ", skipping remaining silence\n", pos / 48 / 20 + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh looking at these changes again (with fresh eyes from @atoppi) there seems to be something broken in the way pos
is updated and handled now. I see how you decided to normalize both pos
and nextPos
, but before pos
depended on i
, while now it doesn't: since tmp
never changes in the for
loop, neither will the value of pos
, making the checks broken. Considering pos
is used as a basis for the av_rescale_q
below, this would likely not insert all the silences we need, in case multiple packets were lost in a row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Updated to increase the pos by 20 * 48
.
7f0a9ed
to
cd57e6c
Compare
hi @lu-zero , Anyway I have observed some differences in the
As you can see the opus file from the PR (
What is the reason of those differences? |
lavf adds the pre-skip, and has a different muxing strategy apparently, the same amount of packets is stored though.
|
Thanks for clarifying, according to the rfc:
AFAIU that means that a player will basically skip (e.g. not play) the first 20 ms (1 packet) of the opus file, while the current pp-rec is generating a file that will play since the very first packet. As of my other point
do you have any comment ? |
It is an estimate
|
@lu-zero I think what @atoppi is mentioning are two separate things, that may or may not be an issue, and that's what we're trying to figure out:
Thanks! |
I set explicitly the packet duration. The original code assumed 20ms packets, I made it explicit. Now the timestamps are identical.
It is the muxing overhead, you can use Your sample has You can remux using
|
Oh I wasn't aware of muxing overhead and that it could be so high, thanks for the clarification and for the helpful snippet!
Do you mean in a new commit? I don't see it yet. |
The push happened a bit later than the message :) |
@lu-zero tried with another recording sample of my voice and got the following with the new pp-rec
Current version does not show any warning. |
Pull and try again please. |
thanks, now everything looks good to me 👍 |
We can merge then! ✌️ |
Thank you for the review :) |
commit 9c9d335 Author: Lionel Nicolas <[email protected]> Date: Thu Jun 10 03:04:43 2021 -0400 Fix streaming plugin mutex unlock when disabling mountpoint (meetecho#2690) commit 2d83e96 Author: Yurii Cherniavskyi <[email protected]> Date: Mon Jun 7 16:02:41 2021 +0300 Fix SIP plugin unhold request docs typo (meetecho#2688) commit 2cd0118 Author: August Black <[email protected]> Date: Mon Jun 7 01:10:49 2021 -0700 minor adjustment to the audiobridge docs (meetecho#2687) commit de2117e Author: nicolasduteil <[email protected]> Date: Tue Jun 1 11:26:29 2021 +0200 fix: [janus_sip] Fix "call_id" property in "missed_call" events (meetecho#2679) commit 9eeeb38 Author: Alessandro Toppi <[email protected]> Date: Mon May 31 15:57:41 2021 +0200 Fix status vector parsing for incoming twcc feedbacks (resolves meetecho#2677). commit 8a25f6e Merge: d3b39b9 394fb48 Author: Alessandro Toppi <[email protected]> Date: Fri May 28 13:29:54 2021 +0200 Merge pull request meetecho#2675 from kmeyerhofer/actions/fix GH Actions, fix variable name commit d3b39b9 Author: Lorenzo Miniero <[email protected]> Date: Fri May 28 11:09:30 2021 +0200 Fixed race condition in VideoRoom commit 394fb48 Author: Kurt Meyerhofer <[email protected]> Date: Thu May 27 14:52:08 2021 -0600 Fixes variable name. commit b45cd37 Author: Lorenzo Miniero <[email protected]> Date: Thu May 27 18:31:55 2021 +0200 Clarify that libnice 0.1.18 is recommended commit 5757a37 Author: Lorenzo Miniero <[email protected]> Date: Thu May 27 17:08:17 2021 +0200 Spatial audio support in AudioBridge via stereo mixing (meetecho#2446) commit 161fe7a Author: Luca Barbato <[email protected]> Date: Thu May 27 15:29:01 2021 +0200 Cleanup avformat-based preprocessors (meetecho#2665) commit 7b010cd Author: lucylu-star <[email protected]> Date: Tue May 25 17:09:40 2021 +0800 Fixed broken simulcast support in VideoCall plugin (meetecho#2671) commit 4ae44a4 Author: nicolasduteil <[email protected]> Date: Mon May 24 17:57:34 2021 +0200 feat: support for custom call-id in subscribe request + add 'call_id' property to subscribe & notify related events (meetecho#2664) commit 4294f20 Author: Lorenzo Miniero <[email protected]> Date: Mon May 24 11:02:48 2021 +0200 Fixed missing macro when using pthread mutexes (fixes meetecho#2666) commit f22ab0d Author: Lorenzo Miniero <[email protected]> Date: Wed May 19 12:03:32 2021 +0200 Fixed warning commit b3f3f17 Author: Alessandro Toppi <[email protected]> Date: Tue May 18 12:10:47 2021 +0200 Remove duplicated flag for fuzzing coverage. commit 4a7560c Author: nu774 <[email protected]> Date: Fri May 14 00:26:36 2021 +0900 janus-pp-rec: support HEVC AP(aggregation packet) (meetecho#2662) commit 5db4be2 Author: Lorenzo Miniero <[email protected]> Date: Wed May 12 17:43:43 2021 +0200 Fixed out of bounds array access commit 69f56f4 Author: nicolasduteil <[email protected]> Date: Tue May 11 14:36:22 2021 +0200 feat: support for SUBSCRIBE expiry (Expires header) in sip plugin (meetecho#2661) commit b047ccf Author: Lorenzo Miniero <[email protected]> Date: Mon May 10 09:33:27 2021 +0200 Fixed types commit f8e8c5e Author: Chris Wiggins <[email protected]> Date: Mon May 10 19:26:45 2021 +1200 RabbitMQ Transport Reconnect Logic (meetecho#2651) commit 280e8e4 Author: Lorenzo Miniero <[email protected]> Date: Fri May 7 12:54:30 2021 +0200 Add per-participant recording options in AudioBridge to join API as well
If you like the set I can convert the other modules likewise and further factor out the common parts.