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

Reduced deprecated code in README.md #323

Merged

Conversation

gertjanal
Copy link
Contributor

@gertjanal gertjanal commented Nov 24, 2016

The current tutorial code has deprecated methods. Replaced them with the new ones. Only one .codec() is left, unsure how to fix.

@saudet
Copy link
Member

saudet commented Nov 26, 2016

The idea is to have some sample translated code that is based on an existing version in C/C++. Here, tutorial01.c happens to be using deprecated functions...

Anyway, not a bad idea to update it. But before we merge this, could you at least remove the try block there? It doesn't do anything useful. Thanks!

@gertjanal
Copy link
Contributor Author

The try block makes sure the finally with the cleanup is always called, even when exiting with return or an throw new Exception

@saudet
Copy link
Member

saudet commented Dec 4, 2016

Yes, your reasoning is good, but we don't need a try-with-resources on AVPacket. We could use one inside SaveFrame() though on the OutputStream. Could you modify it that way? Thanks!

@gertjanal
Copy link
Contributor Author

gertjanal commented Dec 5, 2016

Added try-with-resources to SaveFrame()

Perhaps we don't need the try-with-resources on AVPacket, but the try-finally makes sure this code is always called when done or in case of return or exception:

// Free the RGB image
av_free(buffer);
av_free(pFrameRGB);

// Free the YUV frame
av_free(pFrame);

// Close the codec
avcodec_close(pCodecCtx);

// Close the video file
avformat_close_input(pFormatCtx);

@saudet
Copy link
Member

saudet commented Dec 6, 2016

Yes, this is better, thank you!

One last thing: You're mixing up pFrame and pFrameRGB. Please change it back to how it was originally. Also, don't initialize their fields because they get initialized by the functions called.

@gertjanal
Copy link
Contributor Author

gertjanal commented Dec 7, 2016

Are you sure? The code doesn't work anymore when I don't initialize the fields. Can you copy-paste the code with the intended changes?

(or steal this merge request)

@saudet
Copy link
Member

saudet commented Dec 11, 2016

Well, the way it is now doesn't work for me. Executing the following command:

mvn clean compile exec:java -Dexec.args=http://fate-suite.ffmpeg.org/h264/bbc2.sample.h264

with this pom.xml file:

<project>
    <modelVersion>4.0.0</modelVersion>
    <groupId>org.bytedeco.javacpp-presets.ffmpeg</groupId>
    <artifactId>tutorial01</artifactId>
    <version>1.3</version>
    <properties>
        <exec.mainClass>Tutorial01</exec.mainClass>
        <maven.compiler.target>1.7</maven.compiler.target>
        <maven.compiler.source>1.7</maven.compiler.source>
    </properties>
    <dependencies>
        <dependency>
            <groupId>org.bytedeco.javacpp-presets</groupId>
            <artifactId>ffmpeg-platform</artifactId>
            <version>3.2.1-1.3</version>
        </dependency>
    </dependencies>
</project>

and your Tutorial01.java, I get the following output:

Input #0, h264, from 'http://fate-suite.ffmpeg.org/h264/bbc2.sample.h264':
  Duration: N/A, bitrate: N/A
    Stream #0:0: Video: h264 (High), yuv420p(tv, bt709, top first), 1440x1080 [SAR 4:3 DAR 16:9], 25 fps, 25 tbr, 1200k tbn, 50 tbc
[swscaler @ 0x7efd09909bc0] bad dst image pointers
[WARNING] 
java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.mojo.exec.ExecJavaMojo$1.run(ExecJavaMojo.java:294)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
	at Tutorial012.saveFrame(Tutorial012.java:40)
	at Tutorial012.main(Tutorial012.java:141)
	... 6 more

At the very least please make it work! Thank you

@gertjanal
Copy link
Contributor Author

OOPS! So sorry. I see what you mean with You're mixing up pFrame and pFrameRGB. 😄 pushed working version :)

@saudet
Copy link
Member

saudet commented Dec 23, 2016

Better, now it works :) Thanks

If we really want to use try blocks and stuff though, let's use them properly. The one in the main() method should start before the call to avformat_open_input(), and so all variables need to be declared and initialized at the top, just like in the original C code.

Less importantly, the Exception throws that you added seem conceptually closer to IOException than IllegalStateException, since they can occur often and depend on the input. What do you think?

@saudet
Copy link
Member

saudet commented Feb 4, 2017

Actually, your updated code still uses deprecated API. AFAIU, the following line will not compile starting from the next major version of FFmpeg:

final AVCodecContext pCodecCtx = videoStream.codec();

I couldn't find a properly updated version of that sample in C. Maybe it would be simpler to use an example that comes with FFmpeg itself? This one looks like a good candidate, but it's a bit long:
https://www.ffmpeg.org/doxygen/trunk/demuxing_decoding_8c-example.html
What do you think?

@gertjanal
Copy link
Contributor Author

I think my code suggestion is still a good improvement. However incomplete, it has less deprecated code than the current example.

Also, I have think this takes too much of our time, mostly because I'm not an FFmpeg expert. You can close this pull request if you can think it is incomplete, or merge it if it is an improvement (and create an issue for further improvement)

Thanks for your time and support!

@saudet saudet force-pushed the master branch 3 times, most recently from a1fa858 to 22f9ebd Compare September 24, 2017 11:23
@saudet
Copy link
Member

saudet commented Jan 18, 2018

I agree that we should get rid of deprecated code, but it needs to work. And the purpose of the example in Java is also to provide users with an equivalent translation of an existing example in C/C++.

This example appears to be an updated version of the old one without any deprecated calls:
https://github.com/monday0rsunday/ffmpeg-tutorial/blob/master/002_read_few_frame.c
If you could take this and translate it to Java, that would be awesome!

@saudet saudet merged commit 7a9cd61 into bytedeco:master Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants