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.

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

Create encoder #2

Merged
merged 24 commits into from
Jul 8, 2024
Merged

Create encoder #2

merged 24 commits into from
Jul 8, 2024

Conversation

Noarkhh
Copy link
Collaborator

@Noarkhh Noarkhh commented Jun 26, 2024

No description provided.

Base automatically changed from setup-project to master June 27, 2024 09:33
@Noarkhh Noarkhh marked this pull request as ready for review June 27, 2024 10:33
@Noarkhh Noarkhh requested a review from varsill June 27, 2024 10:33
mix.exs Outdated Show resolved Hide resolved
test/membrane_vpx_plugin/vpx_encoder_test.exs Show resolved Hide resolved
c_src/membrane_vpx_plugin/vpx_encoder.spec.exs Outdated Show resolved Hide resolved
lib/membrane_vpx/encoder/vp8_encoder.ex Outdated Show resolved Hide resolved
lib/membrane_vpx/encoder/vp8_encoder.ex Outdated Show resolved Hide resolved
c_src/membrane_vpx_plugin/vpx_common.c Show resolved Hide resolved
lib/membrane_vpx/encoder/vpx_encoder.ex Outdated Show resolved Hide resolved
lib/membrane_vpx/encoder/vpx_encoder.ex Show resolved Hide resolved
Comment on lines +76 to +78
void get_image_from_raw_frame(vpx_image_t *img, UnifexPayload *raw_frame) {
convert_between_image_and_raw_frame(img, raw_frame, RAW_FRAME_TO_IMAGE);
}
Copy link

Choose a reason for hiding this comment

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

[NIT] do we need that wrapper function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i judged this function not pretty enough and deserving of a wrapper

Copy link

Choose a reason for hiding this comment

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

On the other hand, it adds one layer of abstraction - but you can leave it if you think it looks nicer, the name is rather self-explanatory

const vpx_codec_cx_pkt_t *packet = NULL;

unsigned int frames_cnt = 0, allocated_frames = 1;
UnifexPayload **encoded_frames = unifex_alloc(allocated_frames * sizeof(*encoded_frames));
Copy link

Choose a reason for hiding this comment

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

Is this syntax even allowed? :O What are encoded_frames in sizeof(*encoded_frames)? Does it mean the same as sizeof(UnifexPayload*)? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's just a size of the pointer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i took it from h264_ffmpeg_plugin

Copy link

Choose a reason for hiding this comment

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

Could you set it to UnifexPayload * instead? I have some trust issues with this syntax :D

@Noarkhh Noarkhh requested a review from varsill July 2, 2024 10:34
lib/membrane_vpx/encoder/vp8_encoder.ex Outdated Show resolved Hide resolved

native = Native.create!(state.codec, width, height, pixel_format, state.encoding_deadline)
{flushed_buffers, encoder_ref} =
maybe_recreate_encoder(ctx.pads.input.stream_format, stream_format, state)
Copy link

Choose a reason for hiding this comment

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

Are you sure that it is the old stream format that is available in the ctx?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is this field not the last received stream format on given pad? I checked on the call to handle_stream_format and it was set to nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Indeed, but I wasn't sure whether the most recent stream format received on this pad is the one that has just been received and is handled by this callback, or the previous one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if it's nil on first call i think it's safe to assume that it's the previous one

@Noarkhh Noarkhh requested a review from varsill July 5, 2024 09:10

native = Native.create!(state.codec, width, height, pixel_format, state.encoding_deadline)
{flushed_buffers, encoder_ref} =
maybe_recreate_encoder(ctx.pads.input.stream_format, stream_format, state)
Copy link

Choose a reason for hiding this comment

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

Indeed, but I wasn't sure whether the most recent stream format received on this pad is the one that has just been received and is handled by this callback, or the previous one

encoder will take as long as it needs to produce the best frame possible. Note that
this is a soft limit, there is no guarantee that the encoding process will never exceed it.
If set to `:auto` the deadline will be calculated based on the framerate provided by
incoming stream format. If it's `nil` a fixed deadline of 10ms will be set.
Copy link

Choose a reason for hiding this comment

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

same as in case of the vp8 encoder

@Noarkhh Noarkhh requested a review from varsill July 5, 2024 11:14
Copy link

@varsill varsill left a comment

Choose a reason for hiding this comment

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

🥇

@Noarkhh Noarkhh merged commit f738736 into master Jul 8, 2024
3 checks passed
@Noarkhh Noarkhh deleted the create-encoder branch July 8, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants