issues # 1689 and # 1697 #1734

anotherche · 2021-12-30T18:13:57Z

No description provided.

src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java

saudet · 2022-01-03T00:25:55Z

src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java

@@ -1393,7 +1408,7 @@ public synchronized Frame grabFrame(boolean doAudio, boolean doVideo, boolean do
                        AVRational time_base = video_st.time_base();
                        timestamp = 1000000L * pts * time_base.num() / time_base.den();
                        // best guess, AVCodecContext.frame_number = number of decoded frames...
-                        frameNumber = (int)Math.round(timestamp * getFrameRate() / 1000000L);
+                        frameNumber = (int)Math.floor(timestamp * getFrameRate() / 1000000L);


Why is floor() better than round()?

While fixing #1697 I decided that in all cases when we round some floating results we should simulate result of integer division, so we should round towards zero, like floor does

Do you mean that when we use round(), we might be getting the next frame by mistake, but when we use floor() this doesn't happen, and in the worst case we might be getting the previous frame instead, which is better?

saudet · 2022-01-03T00:27:39Z

src/main/java/org/bytedeco/javacv/Frame.java

@@ -77,10 +77,15 @@

    /** Constants defining data type in the frame*/
    public static enum Type {
+    	UNKNOWN,


Let's make UNKNOWN = -1, making sure VIDEO = 0, and let's also follow the order of FFmpeg, unless there's a good reason not to: AVMEDIA_TYPE_UNKNOWN = -1, AVMEDIA_TYPE_VIDEO, AVMEDIA_TYPE_AUDIO, AVMEDIA_TYPE_DATA, AVMEDIA_TYPE_SUBTITLE, AVMEDIA_TYPE_ATTACHMENT

All right. I swapped data and subtitle by accident.

And I forgot, but we can't set the ordinal() for enum, so for backward compatibility, we shouldn't put UNKNOWN first. Do we even need an UNKNOWN though? It might be problematic for forward compatibility. We could just initialize the field to null like all the rest of the fields. What do you think?

anotherche · 2022-01-03T13:09:39Z

Actually, UNKNOWN is not necessary, I added it just by analogy with original ffmpeg definition. We can use null instead.3 янв. 2022 г. 18:00 пользователь Samuel Audet ***@***.***> написал: @saudet commented on this pull request. In src/main/java/org/bytedeco/javacv/Frame.java:

@@ -77,10 +77,15 @@

/** Constants defining data type in the frame*/ public static enum Type { + UNKNOWN, And I forgot, but we can't set the ordinal() for enum, so for backward compatibility, we shouldn't put UNKNOWN first. Do we even need an UNKNOWN though? It might be problematic for forward compatibility. We could just initialize the field to null like all the rest of the fields. What do you think? —Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>

anotherche · 2022-01-03T13:15:38Z

Yes, that's right. round may give us next or previous integer, while floor is always predictable.3 янв. 2022 г. 17:56 пользователь Samuel Audet ***@***.***> написал: @saudet commented on this pull request. In src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java:

@@ -1393,7 +1408,7 @@ public synchronized Frame grabFrame(boolean doAudio, boolean doVideo, boolean do

AVRational time_base = video_st.time_base(); timestamp = 1000000L * pts * time_base.num() / time_base.den(); // best guess, AVCodecContext.frame_number = number of decoded frames... - frameNumber = (int)Math.round(timestamp * getFrameRate() / 1000000L); + frameNumber = (int)Math.floor(timestamp * getFrameRate() / 1000000L); Do you mean that when we use round(), we might be getting the next frame by mistake, but when we use floor() this doesn't happen, and in the worst case we might be getting the previous frame instead, which is better? —Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>

saudet · 2022-01-04T02:01:52Z

Ok, sounds good! Please update this branch accordingly. Thanks!

anotherche · 2022-01-05T05:51:11Z

Updated the branch. In addition, I realized that my changes in grabFame contained another logical error: I used the pkt.stream_index(-1) technique in doData processing part to guarantee that a new packet is read on the next grabFrame call. However, because of this trick, this Data packet is not unreferenced, which will leak memory. To solve this problem, I abandoned setting the pkt.stream_index to -1, and changed the condition for reading the new package to the following explicit one.
boolean readPacket = (pkt.stream_index() != video_st.index() && pkt.stream_index() != audio_st.index());
I think this is correct, since we need to try to reuse packets related only to the selected audio and video streams, in all other cases we need to read a new packet.

saudet · 2022-01-05T05:59:23Z

We can set stream_index to -1 any time we want, we just need to make sure to call av_packet_unref() before doing that, such as here: https://github.com/bytedeco/javacv/blob/master/src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java#L714-L717
Could you try to do that instead?

anotherche · 2022-01-05T06:09:42Z

But we just have set

// Export the stream byte data for non audio / video frames
frame.data = pkt.data().position(0).capacity(pkt.size()).asByteBuffer();
frame.opaque = pkt;

to return a frame with some data. Now if we call av_packet_unref() after it, what will be in the returned frame then?

saudet · 2022-01-05T07:07:04Z

Ah, yes, ok I see what you mean. So, readPacket is false on the first pass, but it gets set to true on the second pass here:

                if (!readPacket) {
                    readPacket = true;
                    continue;
                }

But you've removed that... why?

anotherche · 2022-01-05T07:49:22Z

Well, because the logic including this part was initially incorrect.
First, if we get to the condition
} else if (doData) {
that means that all the previous if/else if were false, which means various variants, including when a packet (new or old) was from a stream different from the selected audio or video, OR it was from any stream not needed now (say, doAudio or doVideo is false). Then the doData handling part might return frame with data from ANY type of packet, not only data, subs or attachment.
So, I redesigned the condition for doData handling so that it to be explicit, so that it process only not audio and not video packets. In this new logic we need to read new packet if none of the specific if/else triggered - this is now done by the last else:
} else readPacket = true; //current packet is not needed (different stream index required)
And this is now the part which does the work corresponding to the previous code

                if (!readPacket) {
                    readPacket = true;
                    continue;
                }

So, now grabFrame correctly returns data packets if these packets are from data, subs or attachment streams only.
But at the next grabFrame call we must account for two things:

we have to read new packet, because the previously read data packet may trigger the condition
} else if (doData && pkt_stream_ind > -1 && streams.length > pkt_stream_ind && streams[pkt_stream_ind] != AVMEDIA_TYPE_VIDEO && streams[pkt_stream_ind] != AVMEDIA_TYPE_AUDIO) {
again and again
we have to unref previous data packet.

the fist task could be solved by setting stream index to -1 for that packet (as I suggested initially). But then it will not be unref. So we should retain its stream index as initial (this way the second task will be solved automatically if we set readPacket to true somhow - but how?). A possible way could be to check if frame.opaque is equal to the previous packet, and then to read new packet if it is true. But the frame.opaque may be changed in a user code (somewhere between two succeeding grabFrame calls) though. So I decided that the best way will be the change of the initial readPacket condition to the explicit form (we should try to reuse a previous packet for the selected audio and video streams only)

anotherche · 2022-01-05T07:59:04Z

In general, this data processing problem occurs due to the fact that data is handled differently than audio or video. In the case of audio or video, we use the send_packet / receive_frame functions, which themselves suggest whether additional packets need to be read. In the case of data, we do not have such capabilities - we just have to return this data as frame.data and ensure that the new package is read on the next grabFrame call (and the old package is removed as well)

saudet · 2022-01-05T12:03:11Z

In general, this data processing problem occurs due to the fact that data is handled differently than audio or video. In the case of audio or video, we use the send_packet / receive_frame functions, which themselves suggest whether additional packets need to be read. In the case of data, we do not have such capabilities - we just have to return this data as frame.data and ensure that the new package is read on the next grabFrame call (and the old package is removed as well)

Right, the reason to use that block is precisely so that doData behaves like doVideo and doAudio. I suppose it doesn't work when doData is false, yeah. So maybe something like this?

            } else {
                if (!readPacket) {
                    readPacket = true;
                    continue;
                }
                if (doData ...
                }
            }

anotherche · 2022-01-05T17:48:05Z

Yes, that should work correctly as well as the last suggested code in the framenumber branch. The only difference I can see is that processing data packets with

            } else {
                if (!readPacket) {
                    readPacket = true;
                    continue;
                }
                if (doData ...
                }
            }

will always require additional run of the external while(!done) loop to set readPacket=true, while in the case of

boolean readPacket = (pkt.stream_index() != video_st.index() && pkt.stream_index() != audio_st.index());

it is set true from the very beginning for any not video/audio packets. That can be slightly more efficient (but only in case of the data packets processing, which is not so often situation though). So now we have to determine which solution is the best.

saudet · 2022-01-05T22:58:16Z

Actually, something like looks more clean:

            } else if (readPacket && doData && whatever) {
                // ...
            } else if (!readPacket) {
                readPacket = true;
            }

That way, we don't have a continue out of no where, which makes it clear that readPacket is always going to be set and that we're eventually going to either process/return something, or reach the end of the file/stream.

anotherche · 2022-01-06T05:46:01Z

Ok. I agree. But why should we check readPacket in the last else?6 янв. 2022 г. 5:58 пользователь Samuel Audet ***@***.***> написал: Actually, something like looks more clean: } else if (readPacket && doData && whatever) { // ... } else if (!readPacket) { readPacket = true; } That way, we don't have a continue out of no where, which makes it clear that readPacket is always going to be set and that we're eventually going to either process/return something, or reach the end of the file/stream. —Reply to this email directly, view it on GitHub, or unsubscribe.Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>

saudet · 2022-01-07T09:59:42Z

Ok. I agree. But why should we check readPacket in the last else?

We don't need to, we can just force it to true, sure.

anotherche and others added 2 commits December 31, 2021 00:46

# 1689 and # 1697

9f33d36

Merge branch 'master' into framenumber

d53b6e5

saudet reviewed Jan 3, 2022

View reviewed changes

src/main/java/org/bytedeco/javacv/FFmpegFrameGrabber.java Show resolved Hide resolved

saudet reviewed Jan 3, 2022

View reviewed changes

anotherche added 2 commits January 4, 2022 13:56

Frame enum Type reorder and grabFrame readPacket condition fix

84cfe60

Merge remote-tracking branch 'origin/framenumber' into framenumber

2211262

clean doData handling scheme

668633b

saudet added 2 commits January 13, 2022 15:11

Fix formatting

9cf3063

Update CHANGELOG.md

2afd9fb

saudet merged commit 4269832 into bytedeco:master Jan 13, 2022

This was referenced Jan 13, 2022

Very High Latency with FFmpegFrameGrabber.setVideoFrameNumber(lastFrame) #1689

Closed

FFmpegFrameGrabber.setVideoFrameNumber setting unexpected frames #1697

Closed

saudet mentioned this pull request Jul 28, 2022

Improve setting framenumber #1851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issues # 1689 and # 1697 #1734

issues # 1689 and # 1697 #1734

anotherche commented Dec 30, 2021

saudet Jan 3, 2022

anotherche Jan 3, 2022

saudet Jan 3, 2022

saudet Jan 3, 2022

anotherche Jan 3, 2022

saudet Jan 3, 2022

anotherche commented Jan 3, 2022 via email

anotherche commented Jan 3, 2022 via email

saudet commented Jan 4, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 6, 2022 via email

saudet commented Jan 7, 2022 •

edited

Loading

issues # 1689 and # 1697 #1734

issues # 1689 and # 1697 #1734

Conversation

anotherche commented Dec 30, 2021

saudet Jan 3, 2022

Choose a reason for hiding this comment

anotherche Jan 3, 2022

Choose a reason for hiding this comment

saudet Jan 3, 2022

Choose a reason for hiding this comment

saudet Jan 3, 2022

Choose a reason for hiding this comment

anotherche Jan 3, 2022

Choose a reason for hiding this comment

saudet Jan 3, 2022

Choose a reason for hiding this comment

anotherche commented Jan 3, 2022 via email

anotherche commented Jan 3, 2022 via email

saudet commented Jan 4, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 5, 2022

saudet commented Jan 5, 2022

anotherche commented Jan 6, 2022 via email

saudet commented Jan 7, 2022 • edited Loading

saudet commented Jan 7, 2022 •

edited

Loading