gstreamer-java / gst1-java-core

Java bindings for GStreamer 1.x
GNU Lesser General Public License v3.0
188 stars 72 forks source link

MessageProxy message type check confuses extended MessageTypes #202

Closed jkitchenercode closed 3 years ago

jkitchenercode commented 4 years ago

MessageProxy within org.freedesktop.gstreamer.Bus.java has the following method

        public void busMessage(final Bus bus, final Message msg) {
            if ((type.intValue() & msg.getType().intValue()) != 0) {
                callback.callback(bus, msg, null);
            }
        }

This checks whether any bits specified in the mask are present. This approach breaks down for extended MessageTypes, which consist of multiple bits. MessageType.INFO (1 << 3) and MessageType.STREAM_COLLECTION (1 << 31 + 1<<3) will both trigger each others callbacks.

    public void connect(final INFO listener) {
        connect(INFO.class, listener, new BusCallback() {
            public boolean callback(Bus bus, Message msg, Pointer user_data) {
                PointerByReference err = new PointerByReference();
                GSTMESSAGE_API.gst_message_parse_info(msg, err, null);
                GErrorStruct error = new GErrorStruct(err.getValue());
                listener.infoMessage(msg.getSource(), error.getCode(), error.getMessage());
                GLIB_API.g_error_free(err.getValue());
                return true;
            }
        });
    }

When the INFO callback is called for a STREAM_COLLECTION message type, gst_message_parse_info leaves err as null as it is the wrong message type. The null value then gets dereferenced in the next line.

Should the check be something along the lines of

(type.intValue() & msg.getType().intValue()) == msg.getType().intValue()
neilcsmith-net commented 4 years ago

Good catch! I'm not sure the fix shouldn't be just type == msg.getType ?

This is legacy from the previous bindings, and the fact that this was initially a flag upstream.

A PR, ideally with a test, would be great if you're able to?

neilcsmith-net commented 4 years ago

Workaround should be to just use a Message listener and use instanceofon the message. Good question why we're duplicating functionality in Bus and the Message subclasses - should look to merge that in one place.

neilcsmith-net commented 3 years ago

@jkitchenercode out of interest, what triggered this bug for you? Because INFO and STREAM_COLLECTION don't collide. Am putting in a fix for the collisions that do exist now though.

MessageType.INFO (1 << 3) vs MessageType.STREAM_COLLECTION (1 << 31 + 3)

jkitchenercode commented 3 years ago

I raised the bug a little while after identifying the cause, so I must have messed up the message types when writing it up.

Going back over the code, I wasn't actually listening for INFO at the time. I was listening for EOS, ERROR and WARNING, which was enough to clash with most of the extended message types.

Sorry for the delay in replying, I don't monitor this account.