Timecode meta #205

ghost · 2020-08-19T12:23:33Z

I implemented the possibility to get additional metadata from each buffer. Currently, there is only metadata for time codes. In our company, we use these values for navigation in the video (for obtaining exactly the position of showed frame). But there really depends on how the video was created, container type, and codec type. Not all buffers/frames come with attached this kind of metadata.
This is the first time when I wrote some binding over C library, so I hope that I understood correctly how to do it and this extension will be useful also for someone else.

neilcsmith-net · 2020-08-19T13:53:48Z

Thanks for this, and for adding tests!

I'll try and find some free time to review soon. At a glance seems to be in right direction, but can you add links to upstream docs (or code in the case of lowlevel) to the Javadocs as other classes have. This keeps our requirement to document a little lower, and makes for easier review.

Travis tests seem to be failing. Bear in mind that all tests should pass with GStreamer 1.8, currently our baseline. Or are we missing availability of GStreamer elements? Features from higher versions need to be guarded and tests not run. See eg. https://github.com/gstreamer-java/gst1-java-core/blob/master/src/org/freedesktop/gstreamer/Buffer.java#L249 and https://github.com/gstreamer-java/gst1-java-core/blob/master/test/org/freedesktop/gstreamer/PropertyTypeTest.java#L188

neilcsmith-net · 2020-08-24T11:31:59Z

.travis.yml

@@ -5,3 +5,4 @@ addons:
    apt:
        packages:
            - gstreamer1.0-plugins-good
+            - gstreamer1.0-plugins-bad


Is there any feasible way to test this that doesn't require adding gstreamer1.0-plugins-bad? We've generally tried to keep that out of test requirements.

I just need these plugins not for production but just for tests. There is timecodestamper which can be used for testing timecode metadata. But true is that timecodestamper is available since 1.10.

Yes, just wondering if there's any way to do test differently without this element? If we can keep the test requirements to plugins-good, I'd be happier.

neilcsmith-net · 2020-08-24T11:32:41Z

src/org/freedesktop/gstreamer/Buffer.java

@@ -142,7 +141,7 @@ public long getDecodeTimestamp() {
     * Set the decode timestamp of the Buffer
     *
     * @param val a long representing the timestamp or
-     * {@link ClockTime#NONE} when the timestamp is not known or relevant.
+     *            {@link ClockTime#NONE} when the timestamp is not known or relevant.


Please keep whitespace changes out of this where you can, thanks.

Sorry about that, I forgot to uncheck the checkbox for formatting code before commit.

neilcsmith-net · 2020-08-24T11:39:44Z

src/org/freedesktop/gstreamer/Buffer.java

+     *
+     * @return return time code (SMPTE) for current buffer
+     */
+    public GstVideoTimeCodeMeta getVideoTimeCodeMeta() {


I'm unsure about the API here. Maybe we need a getMeta() method that takes a type, which should probably be specified by Java class. Will check upstream docs.

The type here should be VideoTimeCodeMeta - the Gst prefix should only be used in lowlevel package. Helps to make sure we're not exposing things we shouldn't be!

neilcsmith-net · 2020-08-24T11:41:35Z

src/org/freedesktop/gstreamer/lowlevel/GstMetaApi.java

+import org.freedesktop.gstreamer.timecode.GstVideoTimeCodeFlags;
+
+/**
+ * @author Jokertwo


Please remove the author annotation. Add license headers and add your real name in copyright lines. Also feel free to add your name in a copyright line on modified files if you wish.

neilcsmith-net · 2020-08-24T11:48:42Z

src/org/freedesktop/gstreamer/meta/GstMetaData.java

+ * Gst meta data GTypes
+ * @author Jokertwo
+ */
+public enum GstMetaData {


I'm not sure enum seems right here - would expect probably Java types like messages, events and queries. Thoughts?

neilcsmith-net · 2020-08-24T11:53:34Z

src/org/freedesktop/gstreamer/meta/GstMetaInfo.java

+ * @author Jokertwo
+ * @see <a href="https://gstreamer.freedesktop.org/documentation/gstreamer/gstmeta.html?gi-language=c#GstMetaInfo">GstMetaInfo</a>
+ */
+public class GstMetaInfo extends MiniObject {


Should be called MetaInfo.

neilcsmith-net · 2020-08-24T12:04:35Z

src/org/freedesktop/gstreamer/meta/GstMeta.java

+/**
+ * @author Jokertwo
+ */
+public class GstMeta implements NativeObject.TypeProvider {


Should be called Meta. Should specific Meta extend from this? What is the Meta hierarchy? Java class hierarchy should generally match with GType hierarchy.

The Type.Provider implementation should probably be nested, like Message.Types

neilcsmith-net · 2020-08-24T12:06:48Z

src/org/freedesktop/gstreamer/meta/GstVideoTimeCodeMeta.java

+ * @author Jokertwo
+ * @see <a href="https://docs.gstreamer.com/documentation/video/gstvideometa.html?gi-language=c#GstVideoTimeCodeMeta">GstVideoTimeCodeMeta</a>
+ */
+public class GstVideoTimeCodeMeta extends MiniObject {


VideoTimeCodeMeta. Is this really a MiniObject?

I do not know, how to resolve if is it MiniObject or not?

Documentation

Source code

neilcsmith-net · 2020-08-24T12:09:33Z

src/org/freedesktop/gstreamer/timecode/GstVideoTimeCode.java

+ * @author Jokertwo
+ * @see <a href="https://docs.gstreamer.com/documentation/video/gstvideotimecode.html?gi-language=c#GstVideoTimeCode">GstVideoTimeCode</a>
+ */
+public class GstVideoTimeCode extends MiniObject {


VideoTimeCode. Really a MiniObject? Do we really need a separate package for this?

I do not know, how to resolve if is it MiniObject or not?

Documentation

Source code

You should see a GstMiniObject as first element of the struct - eg. https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/master/gst/gstbuffer.h#L265 Or you could try calling various bits of the MiniObject API on them, which will likely crash! 😄 Hence need to be careful here. Likely that you'll want to extend NativeObject directly if anything - check out other classes that directly extend that.

neilcsmith-net · 2020-08-24T12:10:30Z

src/org/freedesktop/gstreamer/timecode/GstVideoTimeCodeConfig.java

+ * @author Jokertwo
+ * @see <a href="https://docs.gstreamer.com/documentation/video/gstvideotimecode.html?gi-language=c#GstVideoTimeCodeConfig">GstVideoTimeCodeConfig</a>
+ */
+public class GstVideoTimeCodeConfig extends MiniObject {


VideoTimeCodeConfig. Really a MiniObject?

Fix comments from Neil

ghost · 2020-08-26T15:59:29Z

I hope that I fixed all your comments

neilcsmith-net · 2020-08-26T16:02:03Z

Thanks. I'll take a look as soon as I can - probably won't be able to for at least a week.

neilcsmith-net · 2020-09-15T10:41:43Z

Sorry this is taking so long - few other projects going through releases and hitting some teething problems. Have scheduled some time for next week to fully review. Thanks for your patience!

neilcsmith-net · 2020-09-23T09:04:35Z

Thanks for looking at this. I still have some issues with it as is. I'm still not sure about the package structure, a few of the method names, the license headers are in the wrong place and mixed up with what should be Javadocs, and I'd still like to get tests working without plugins bad if at all possible.

I'm going to change the base branch for this and merge into a specific branch here to work on together before it hits master.

neilcsmith-net · 2020-09-30T16:55:53Z

@aveco-devel I've started reviewing through, and working out a task list for merging this code. Unfortunately, I hadn't realised quite how much this was exposing lowlevel package types (eg. GType) and JNA types (eg. Pointer). Neither are allowed in our public API as a rule. Feel free to join in the discussion on the linked thread if you wish, and thanks for your work to date.

Jokertwo added 2 commits August 19, 2020 13:56

Implement timecode metadata

e0efa21

added test for timecode metadata

9844214

Jokertwo and others added 4 commits August 24, 2020 12:26

Add metadata info structure and objects

378b875

Added missing documentation, and checking of version

cbafd49

added checking version in tests

da3e198

added a check for backward compatibility

597f6f8

neilcsmith-net requested changes Aug 24, 2020

View reviewed changes

Jokertwo and others added 8 commits August 24, 2020 16:07

added licence

75de65a

fix build

ef9a6a7

replace mini object by native object

2492223

rename by naming conventions

530bc71

created factory method for creating metadata classes

3688adf

removed unnecessary class for register meta types

411ce57

change enum to flags

3c66be2

Merge pull request #3 from aveco-devel/fix-comments

e2f7c08

Fix comments from Neil

neilcsmith-net added the enhancement label Aug 26, 2020

neilcsmith-net added this to the 1.3 milestone Aug 26, 2020

neilcsmith-net changed the base branch from master to meta September 23, 2020 09:05

neilcsmith-net merged commit fae0b5e into gstreamer-java:meta Sep 23, 2020

neilcsmith-net mentioned this pull request Sep 23, 2020

Support for GstMeta (partial) #208

Merged

Uh oh!

Timecode meta #205

Timecode meta #205

Uh oh!

Conversation

ghost commented Aug 19, 2020

Uh oh!

neilcsmith-net commented Aug 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Aug 24, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Aug 24, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 26, 2020

Uh oh!

neilcsmith-net commented Aug 26, 2020

Uh oh!

neilcsmith-net commented Sep 15, 2020

Uh oh!

neilcsmith-net commented Sep 23, 2020

Uh oh!

neilcsmith-net commented Sep 30, 2020

Uh oh!

Uh oh!

ghost Aug 24, 2020 •

edited by ghost

Loading

ghost Aug 24, 2020 •

edited by ghost

Loading