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

Provide output pixel format #24

Merged
merged 4 commits into from
Jan 3, 2025

Conversation

gBillal
Copy link
Collaborator

@gBillal gBillal commented Jan 3, 2025

Fix #23

Later on I'll extract the converter to be standalone (to be used as an Elixir module independent of the decoder, it may be useful)

Copy link
Member

@mickel8 mickel8 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

There are two small things we should fix before merging: av_get_pix_fmt can receive inccorect data and we should update docs for Xav.Decoder.new/2 as it states that data is always returned in RGB format.

I also wonder if we should somehow let people know (besides changelog) that rgb is no longer the default out format 🤔

The list of accepted formats are all `ffmpeg` pixel formats. For a complete list run:

```sh
ffmpeg -pix_fmts
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate instruction how to get a list of possible formats!

@@ -16,7 +17,7 @@ struct Decoder {

struct Decoder *decoder_alloc();

int decoder_init(struct Decoder *decoder, const char *codec);
int decoder_init(struct Decoder *decoder, const char *codec, const char* format);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, and could we do the same in decoder.c?

Suggested change
int decoder_init(struct Decoder *decoder, const char *codec, const char* format);
int decoder_init(struct Decoder *decoder, const char *codec, const char* out_format);

Comment on lines +84 to +86
if (out_pix_fmt == AV_PIX_FMT_NONE) {
return xav_nif_video_frame_to_term(env, frame);
}
Copy link
Member

Choose a reason for hiding this comment

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

I thnk we should distinct between a case where someone didn't pass out_format and someone passed out_format but it was incorrect.

In decoder_init, we should check whether out_format is nil. If yes, set out_format field in decoder to null. If not, try to call av_get_pix_fmt. If av_get_pix_fmt returns AV_PIX_FMT_NONE, return an error.

@mickel8
Copy link
Member

mickel8 commented Jan 3, 2025

Later on I'll extract the converter to be standalone (to be used as an Elixir module independent of the decoder, it may be useful)

Would be great!

@gBillal
Copy link
Collaborator Author

gBillal commented Jan 3, 2025

I also wonder if we should somehow let people know (besides changelog) that rgb is no longer the default out format 🤔

Perhaps we can add a warning log if the out_format is nil.

@mickel8
Copy link
Member

mickel8 commented Jan 3, 2025

Perhaps we can add a warning log if the out_format is nil.

Let's leave it as it is. Such warning would be too noisy imo

@mickel8 mickel8 merged commit 13fc3b0 into elixir-webrtc:master Jan 3, 2025
@gBillal gBillal deleted the provide-output-pixel-format branch January 20, 2025 12:44
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.

Allow providing output pixel format for decoder
2 participants