Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add FFmpegHlsOutputBuilder for HLS user #322

Merged
merged 20 commits into from
Aug 8, 2024

Conversation

van1164
Copy link
Contributor

@van1164 van1164 commented Mar 19, 2024

Hi, @bramp

I've been using the HLS format of ffmpeg with .addExtraArgs() . But I think it's too uncomfortable

So, I made an FFmpegHlsOutputBuilder for HLS user, what do you think?

Thank You.

Before

image

After

image

Copy link
Owner

@bramp bramp left a comment

Choose a reason for hiding this comment

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

Awesome, I think this will be super useful.

I added a few comments. Please take a look.

Also adding @Euklios as he has plans to change FFmpegOutputBuilder, and I want to make sure this doesn't conflict.

src/main/java/net/bramp/ffmpeg/builder/FFmpegBuilder.java Outdated Show resolved Hide resolved
* @return {@link FFmpegHlsOutputBuilder}
*/
public FFmpegHlsOutputBuilder setHlsListSize(int size){
checkArgument(size>0, "Size cannot be less than 0.");
Copy link
Owner

Choose a reason for hiding this comment

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

Size cannot be less than or equal to zero.

Maybe easier to write, Size must be positive (e.g like here:

checkArgument(bit_rate > 0, "bit rate must be positive");
)

Copy link
Contributor Author

@van1164 van1164 Mar 20, 2024

Choose a reason for hiding this comment

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

I understand, but according to the ffmpeg document

hls_list_size size
Set the maximum number of playlist entries. If set to 0 the list file will contain all the segments. Default value is 5.

So I wrote it like that. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, but your code checks it's greater than zero, where your comment says can't be less than zero, so there is a mismatch there.

Seems like it should be checkArgument(size >= 0,...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I made a mistake 😥. Thank you

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct, size == 0 is basically required for VOD, I would add a test for this scenario specifically as well.

@Euklios
Copy link
Collaborator

Euklios commented Mar 20, 2024

I just noticed and haven't looked at the implementation yet, but I love the idea.
@van1164 Is there a difference between using the HLS-specific flags and generic alternatives like -segment_list_size, -segment_start_number, and -segment_format in combination with -segment_list_type m3u8?

@van1164
Copy link
Contributor Author

van1164 commented Mar 20, 2024

I just noticed and haven't looked at the implementation yet, but I love the idea. @van1164 Is there a difference between using the HLS-specific flags and generic alternatives like -segment_list_size, -segment_start_number, and -segment_format in combination with -segment_list_type m3u8?

Thank you for your response. @Euklios 😁

I don't think there's any difference in the outcome in the simple case.
But I think HLS users can handle things less complicatedly.

@van1164
Copy link
Contributor Author

van1164 commented Mar 21, 2024

Hi, @bramp @Euklios

I've thought of several ways, and I think it's better to make AbstractOutputBuilder() between multiple OutputBuilder() (including default OutputBuilder) and AbstractFFmpegStreamBuilder.

I have committed to completing the development, and pass existing tests and newly added tests.

and if you have any better opinions, I am always welcome.

Please check it. Thank you. 😁

@van1164 van1164 requested review from bramp and Euklios April 4, 2024 12:06
@bramp
Copy link
Owner

bramp commented Apr 4, 2024

Ok, the changes all look good. Thanks a lot of the work.

Can we resolve the conflict, then we'll run the github test actions, and we are good to go.

@van1164
Copy link
Contributor Author

van1164 commented Apr 24, 2024

Hi, @bramp @Euklios
I've solved all the conflicts 😁

@van1164
Copy link
Contributor Author

van1164 commented Jun 28, 2024

HI, @bramp
Could I inquire about the progress?

@Euklios Euklios merged commit a6e8213 into bramp:master Aug 8, 2024
4 checks passed
@Euklios
Copy link
Collaborator

Euklios commented Aug 8, 2024

@van1164 Sorry for the wait, I just merged your PR.

Note: I created #337 to fix a small error that I only caught when doing some local testing.
Essentially, the format you were setting in addHlsOutput was setting the input format.

@van1164
Copy link
Contributor Author

van1164 commented Aug 8, 2024

Thank reply @Euklios

And can I know about the future update plan?
I would like to continue to contribute to this repo update in the future.😊

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants