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

Add enum for video and audio codecs. #321

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Add enum for video and audio codecs. #321

merged 7 commits into from
Apr 4, 2024

Conversation

van1164
Copy link
Contributor

@van1164 van1164 commented Mar 18, 2024

@bramp

Hi. Thank you for using your library well.

I want to add Enum to make it easier to identify and add codecs in IDE.

like

new FFmpegOutputBuilder().setFilename("output.mp4").setVideoCodec(VideoCodec.H264.codec)

What do you think?

Please reply.

add enum to make it easier for developers to access codecs.
@van1164 van1164 changed the title Added enum for video and audio codecs. Add enum for video and audio codecs. Mar 18, 2024
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.

So this seems fine in principle, but I do have a couple of comments/concerns

  1. How is the list of codecs generated? How can we easily keep it updated?

  2. Not all the codecs will be available on the target system, and I'd prefer folks call FFmpeg.codecs() instead to figure out what's available. So maybe if these enums are commented to explain that these may not be available, and the caller should cross check with codecs() or run the risk of their command failing.

  3. I wonder if this should be a list of Codec objects, instead of Strings.

@van1164
Copy link
Contributor Author

van1164 commented Mar 19, 2024

Thank you for the reply.

@bramp

  1. The code generation was implemented in a similar way to codecs() with python. If you like the Enum method, I will write a safer code.

  2. That's what I've been thinking. It's difficult for developers to know which codecs are available in IDE only with the codecs() function. I'm still thinking about how to leave comments on codecs that are not possible for each target system. So far, we still have to cross check through codecs(), but I think it's also important to let them know what codecs FFmpeg provides.

  3. If you can develop it like the picture below, I'm thinking of considering any way. Recommendations are also welcome.
    image

@bramp
Copy link
Owner

bramp commented Mar 19, 2024

Ok cool. Thanks for the responses, so

  1. If you could supply the python / simple java app, we can leave instructions for folks to run/update this in future.

  2. Ok, so if you would add some documentation to the enum, I think that'll be sufficient.

  3. Yes, that actually looks helpful when you show it within a IDE. So keeping it a Enum with a String codec property seems reasonable

@Euklios do you have any opinions on the above.

@van1164
Copy link
Contributor Author

van1164 commented Mar 19, 2024

@bramp Thanks for the responses 😁

Would document be okay as below? If you like it, I'll commit

image

, and this is the python code for the above results.

import subprocess
import re

CODECS_REGEX = re.compile("^ ([.D][.E][VASD][.I][.L][.S]) (\S{2,})\s+(.*)$")

def removeLeadingNumbers(text):
    index = 0
    while index < len(text) and text[index].isdigit():
        index += 1
    return text[index:]

def writeCodec(m,codec):
    document = "\t"+"/**"+ m.group(3).rstrip()  +"*/\n"
    enumCode = "\t" +removeLeadingNumbers(m.group(2).replace(".","_")).upper() +'("'+  m.group(2) +'"),' +'\n'
    codec.write(document)
    codec.write(enumCode)


output = subprocess.check_output("ffmpeg -codecs", shell=True).decode('utf-8')

print(output) 

output = output.split("\n")

videoCodec = open("VideoCodec.java", 'w')
audioCodec = open("AudioCodec.java", 'w')

videoCodec.write(
"""package net.bramp.ffmpeg.builder;

public enum VideoCodec {
""")
audioCodec.write(
"""package net.bramp.ffmpeg.builder;

public enum AudioCodec {
""")
for item in output:
    m = CODECS_REGEX.match(item)
    if not m : continue

    lst = item.split()
    if 'A' in m.group(1):
        writeCodec(m,audioCodec)

    if 'V' in m.group(1):
        writeCodec(m,videoCodec)


videoCodec.write("""   
    ;
    final String codec;

    VideoCodec(String codec) {
        this.codec = codec;
    }

    @Override
    public String toString() {
        return this.codec;
    }
}
""")

audioCodec.write("""
    ;
    final String codec;

    AudioCodec(String codec) {
        this.codec = codec;
    }

    @Override
    public String toString() {
        return this.codec;
    }
}

""")
videoCodec.close()
audioCodec.close()

Thank you for reading 😋

@bramp
Copy link
Owner

bramp commented Mar 19, 2024

Thanks, that documentation looks good. I also think a comment on the enum itself saying to use FFmpeg.codec() for a accurate list for your deployment.

As for the python I think it should be checked in, under /tools/generate_codec_enum.py or similar. Then we can update the README to mention it.

@van1164
Copy link
Contributor Author

van1164 commented Mar 19, 2024

@bramp
Thank you for your advice 😁.

I committed document and enum generator python code for enum.

Thank you.

@Euklios
Copy link
Collaborator

Euklios commented Mar 20, 2024

@bramp @van1164

An enum would convey a notion of "completeness" and "correctness"; neither can be guaranteed.
Wouldn't a class containing public static final fields convey better that this is a common set of codecs?

@bramp
Copy link
Owner

bramp commented Mar 20, 2024

Oh, that is a good point. So this maybe should be more like the AUDIO_SAMPLE_* constants. They are not complete, but are handy for folks use.

So yes, I think a public static final fields is better.

@Euklios
Copy link
Collaborator

Euklios commented Mar 20, 2024

So that we're on the same page:

My thought was something like

class VideoCodecs {
  public static final ____ HEVC = new ____

Keeping it out of FFmpeg is a good idea.
As for the type, I don't have a preference, but we could reuse the existing Codec class. Do you have any thoughts on that one?

@bramp
Copy link
Owner

bramp commented Mar 20, 2024

ah yes, keeping it out of FFmpeg sounds good to me. We could add it to Codec, but I suspect we are not making a full Codec instance here. So maybe two new class VideoCodec, and AudioCodec

@van1164
Copy link
Contributor Author

van1164 commented Mar 21, 2024

@bramp @Euklios

I changed the enum to a codec class with a string value. I also committed the python code that generates it.

How about applying it like this?

When using it, the picture is shown below
image

@bramp
Copy link
Owner

bramp commented Mar 26, 2024

The Build fails now with this error:

Error:  /home/runner/work/ffmpeg-cli-wrapper/ffmpeg-cli-wrapper/src/main/java/net/bramp/ffmpeg/builder/AudioCodec.java:97: error: bad HTML entity
Error:      /**ADPCM IMA Simon & Schuster Interactive*/

This seems a little strict, but I suspect it want's you to change & to &

@@ -0,0 +1,69 @@
import subprocess
Copy link
Owner

Choose a reason for hiding this comment

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

A quick comment at the top saying how to run this tool, and a pointer to the file it generates, would be useful.

Imagine a year from now, someone coming in code, having to run this.

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 didn't know there would be a problem with & at javadoc . 😅 I modified it in codec files and generator.

And I added a description at the top of the generator.

Thank you.

@van1164 van1164 requested a review from bramp April 4, 2024 09:41
@bramp bramp merged commit 6ecaef8 into bramp:master Apr 4, 2024
4 checks passed
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.

3 participants