Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed broken faststart when postprocessing AV1 recordings #3317

Merged
merged 4 commits into from
Jan 22, 2024
Merged

Fixed broken faststart when postprocessing AV1 recordings #3317

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a164850
Fixed broken faststart when postprocessing AV1 recordings
Dec 26, 2023
6cb51bf
Removed useless filename
Jan 9, 2024
1386b2c
Added missing g_free
Jan 22, 2024
fe0a698
Set url to null
Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/postprocessing/pp-av1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you're going through a sprintf here. Can't you just g_strdup(destination)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, thanks! Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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_HEADER flag was already there for H264 and H265 before 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@corthiclem corthiclem Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


vStream = janus_pp_new_video_avstream(fctx, AV_CODEC_ID_AV1, max_width, max_height);
if(vStream == NULL) {
JANUS_LOG(LOG_ERR, "Error adding stream\n");
Expand Down