Fixed broken faststart when postprocessing AV1 recordings #3317
Conversation
|
Thanks for your contribution, @corthiclem! Please make sure you sign our CLA, as it's a required step before we can merge this. |
src/postprocessing/pp-av1.c
Outdated
| @@ -55,6 +55,10 @@ int janus_pp_av1_create(char *destination, char *metadata, gboolean faststart, c | |||
| return -1; | |||
| } | |||
|
|
|||
| char filename[1024]; | |||
| snprintf(filename, sizeof(filename), "%s", destination); | |||
| fctx->url = g_strdup(filename); | |||
There was a problem hiding this comment.
I don't understand why you're going through a sprintf here. Can't you just g_strdup(destination)?
There was a problem hiding this comment.
You're right, thanks! Fixed.
There was a problem hiding this comment.
Don't forget the g_free(fctx->url); that the commit you referenced includes, or you'll be introducing a leak. I see that patch also did a few other things for each codec, like setting CODEC_FLAG_GLOBAL_HEADER: are those other changes unneeded for AV1 processing?
There was a problem hiding this comment.
If I am not wrong, the avformat_free_context function is used in pp_av1.c when closing the file, which already frees the memory allocated (according to ffmpeg documentation), so there is no need for g_free(fctx->url).
I think the patch just removed unused comments and that CODEC_FLAG_GLOBAL_HEADER flag was already there for H264 and H265 before the patch I mentionned. I can try to add the global header flag but I am not familiar enough with libav to know what will be the impact on the resulting video. I think it's not used either for WebM postprocessing.
There was a problem hiding this comment.
If I am not wrong, the
avformat_free_contextfunction is used inpp_av1.cwhen closing the file, which already frees the memory allocated (according to ffmpeg documentation), so there is no need forg_free(fctx->url).
We allocate it ourselves with a g_strdup which is not compatible with the av_freep that libavformat does. You have to free it yourself with g_free and then set the pointer to NULL, which is what the other patch did.
I think the patch just removed unused comments and that
CODEC_FLAG_GLOBAL_HEADERflag was already there forH264andH265before the patch I mentionned
I don't think it's on by default, and IIRC for H.264/H.265 it's needed for seeking and I can't remember what else. We're only saving AV1 to MP4 so WebM is irrelevant here.
There was a problem hiding this comment.
When I add g_free before avformat_free_context, I do get a free(): double free detected in tcache 2 but it seems to be working fine if I add it after. I updated the pull request.
What I could do is use av_strdup instead of g_strdup I guess.
There was a problem hiding this comment.
That's because you ignored part of my previous message 🙂
You have to free it yourself with g_free and then set the pointer to NULL, which is what the other patch did.
You didn't set the pointer to NULL, so avformat_free_context tries to free something that doesn't exist anymore.
There was a problem hiding this comment.
Oh sorry, Monday mornings ... 😅
Will fix.
Concerning the global flag headers, I suggest doing it in another pull request as it requires a bit more work (creating a codec context and switching to a codec parameter which is not yet done for in pp-av1)
There was a problem hiding this comment.
Sure, no problem! I asked about the global flag header only because it was in the other patch, and so I wondered if it could be needed for AV1 too: it may very well be that it isn't, I rarely work with any AV1 recording at all so I don't have experience with them.
There was a problem hiding this comment.
I'm new to AV1 recording, so I don't know if the global flag header helps but I'll take a deeper look at this as soon as I have time.
|
Thanks for the fixes! If you can confirm it works for your use case, now, I'll merge 👍 |
|
Yes, all good on my side :) |
|
Ack, merging then! I'll backport to |
This fixes #3315. It replicates what has been done in adea15f but for AV1 post-processing. (And re-opens #3316 without typo in the branch name)