Corrupted picture grabbed after setTimestamp() invocation

[egorapolonov]

Hi @saudet ,

Looks like last fix #870 breaks down picture grabbing functionality.
I mean if you invoke setTimestamp() and jump unluckily to the frame which contains audio data only then frameGrabbed flag will be true in any case. Let's reproduce using code below

import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;

import javax.imageio.ImageIO;

import org.bytedeco.javacv.FFmpegFrameGrabber;
import org.bytedeco.javacv.Java2DFrameConverter;

public class PictureAnalyzer {
    
    public static void main(String[] args) throws IOException {
        extractPicture(9201000, "invalid");
        extractPicture(9000000, "correct");
    }
    
    public static void extractPicture(long timestampInMicros, String suffix) throws IOException {
        File outputFile = new File("picture_" + timestampInMicros + "_" + suffix + ".jpg");
        try (FFmpegFrameGrabber video = new FFmpegFrameGrabber("https://www.dropbox.com/s/gilbbjjhft4tzxn/00A8.mp4?dl=1")) {
            video.start();
            video.setTimestamp(timestampInMicros);
            BufferedImage image = new Java2DFrameConverter().convert(video.grabImage());
            ImageIO.write(image, ".jpg", outputFile);
        }
    }
    
}

First timestamp we are lucky and picture is okay
picture_9000000_correct.jpg

Another timestamp picture is broken
picture_9201000_invalid.jpg

Some fix - for correct picture grabbing after setTimestamp() invocation - force setup doAudio to false

public class PictureGrabber extends FFmpegFrameGrabber {

        public PictureGrabber(String filename) {
            super(filename);
        }
        
        @Override
        public Frame grabFrame(boolean doAudio, boolean doVideo, boolean processImage, boolean keyFrames)
                throws Exception {
            return super.grabFrame(false, doVideo, processImage, keyFrames);
        }
        
    }

It would be nice to have flagging about last grabbed frame - was there audio frame grabbed or video. Whtat do you think about it?

[saudet]

If you're not interested in audio frames, you should be calling grabImage() after setTimestamp() and this problem won't happen.

[egorapolonov]

Hi @saudet ,
That's why I'm asking. Take a look at the code example again, especially this area:

video.setTimestamp(timestampInMicros);
BufferedImage image = new Java2DFrameConverter().convert(video.grabImage());

I really called grabImage after setTimestamp and got corrupted picture.
I analyzed source code of FFmpegFrameGrabber.grabFrame() and see that this check

if (doVideo && frameGrabbed) {

doesn't know what kind of frame was grabbed

[saudet]

Ah, I see. Could you send a pull request with the fix? Thanks for reporting!

[egorapolonov]

Okay, I'll try to make tidy refactoring of grabFrame() and setTimestamp() methods

[saudet]

Thanks! I think we just need to set frameGrabbed to true only when we grab an image right? And it would be a good idea to rename it to "imageGrabbed" instead as well..

[saudet]

Thanks for starting work on that! Are you going to send a pull request? BTW, your code doesn't work because it returns audio frames with frame.samples == null. Also, we don't need to have a separate enum for the type of frame. If frame.images != null, then we have a video frame, and when frame.samples != null we have an audio frame.

[saudet]

I've fixed the issue in the commit above. Let me know if you still have problems with this! Thanks

[egorapolonov]

Hi @saudet ,
I was busy and didn't check mailbox. Thanks for your work and nice comments!
I haven't created pull-request due to bug you described above. Now, as I can see I should stop working on my code and just test your fix. I'll test and let you know about the results

[egorapolonov]

Hi @saudet,
I checked pictures and found one issue.
Try this code

public static void testPictures() throws IOException {
        long timestampInMicros = 9000000;
        String suffix = "test";
        for(int increment=0; increment <100_000_000;increment+=1_000_000) {
            try (FFmpegFrameGrabber video = new FFmpegFrameGrabber("https://www.dropbox.com/s/gilbbjjhft4tzxn/00A8.mp4?dl=1")) {
                video.start();
                timestampInMicros += increment;
                System.out.println("timestampInMicros = " + timestampInMicros);
                System.out.println("video.getLengthInTime() = " + video.getLengthInTime());
                File outputFile = new File("picture_" + timestampInMicros + "_" + suffix + ".jpg");
                video.setTimestamp(timestampInMicros);
                BufferedImage image = new Java2DFrameConverter().convert(video.grabImage());
                ImageIO.write(image, "jpg", outputFile);
                System.out.println("video.getTimestamp() = " + video.getTimestamp());
            }
        }
    }

Stacktrace looks like this:

timestampInMicros = 145000000
video.getLengthInTime() = 132612200
Exception in thread "main" java.lang.IllegalArgumentException: image == null!
	at javax.imageio.ImageTypeSpecifier.createFromRenderedImage(ImageTypeSpecifier.java:925)
	at javax.imageio.ImageIO.getWriter(ImageIO.java:1592)
	at javax.imageio.ImageIO.write(ImageIO.java:1520)

UPD: timestamp > video length. Ignore this comment!

[egorapolonov]

@saudet , sorry. There is my mistake. I set timeStamp > video length.
So, I tested hudreds of video, everything worked fine! Thank you!

[saudet]

Fix included in JavaCV 1.4.1! Thanks again for reporting and for testing this. Please let me know if you still have any issues though.

[egorapolonov]

Hi @saudet ,
Nice to read such great news!
I'm planning to migrate to 1.4.1 in a few weeks. I would inform you if I found something.
Thank you!