[gstreamer-bugs] [Bug 631200] flacparse: major performance improvements

Sat Oct 23 02:05:37 PDT 2010

https://bugzilla.gnome.org/show_bug.cgi?id=631200
  GStreamer | gst-plugins-bad | git

--- Comment #36 from Felipe Contreras <felipe.contreras at gmail.com> 2010-10-23 09:05:33 UTC ---
(In reply to comment #35)
> (In reply to comment #32)
> > (In reply to comment #29)
> > > (From update of attachment 171597 [details] [details] [details])
> > > Except the GValue initializations this is fixed now. I don't see what's wrong
> > > with these initializations and GLib and GStreamer is doing that *everywhere*
> > 
> > That doesn't make it right.
> > 
> > See bug #577231.
> 
> Everything after the first 0 will be initialized to 0 too, that warning only
> means that you didn't explicitely initialize the remaining struct members.

I know that, but -Wextra is pretty essential.

> (In reply to comment #33)
> > (In reply to comment #31)
> > > (From update of attachment 171603 [details] [details] [details])
> > > I don't see anything wrong with this code either.
> > > 
> > > a) Every list node always contains a valid buffer reference
> > 
> > Not true, there are some error conditions where this doesn't happen.
> 
> Which cases?

I don't know, invalid clips?

> > I know because I saw the warnings of gst_mini_object_unref(), which why I
> > bothered writing the patch.
> > 
> > BTW. gst_mini_object_unref has a silly API, should be:
> > void gst_mini_object_unref(void *mini_object);
> 
> There's a gst_buffer_unref() for the correct parameter type.

Yeah, but that's why g_object_unref accepts a void, so you don't need the
correct parameter type.

> > And should accept NULL, that would make it really useful, similar to free(),
> > and g_object_unref().
> 
> g_object_unref() does not accept NULL, neither does free() (in general).
> g_free() accepts NULL, yes.

1) g_object_unref() receives a void
2) free accepts NULL

See the C standard on "7.20.3.2 The free function":
If ptr is a null pointer, no action occurs.

> (In reply to comment #34)
> > For the record:
> > % git log --author=felipe.contreras -- gst/audioparsers
> > 
> > Shows basically nothing. Way to incentivize contributors.
> 
> I committed two of your patches which could be included. The caps fix and the
> picture parsing fix.
> I rejected other patches because they were not necessary (as explained above)
> or the large rewrite of the parsing because you added unrelated changes and
> also used C99 features everywhere.
> 
> Instead I've done the rewrite of the parsing myself based on your idea, which
> was done faster than splitting your patches, cleaning things up, etc. But if
> you look at the commit message of that change you'll see your name.

That's not enough. I would have expected you to add review comments so that I
could do the changes and remain the author of the patches, that would give the
appropriate attribution.

Now you remain as the author of the patch. Even though I'm mentioned on the
commit message as making the "suggestion", nobody will really see my name, not
on the log, or statistics of any kind:

https://www.ohloh.net/p/gstreamer/contributors

> > I have more luck contributing patches to the linux kernel, where they:
> >  a) actually review the patches
> 
> Not sure why you think nobody reviewed your patches

By "review", I mean go through the code, and as you go, make comments about it.

Not keep the comments to yourself, and afterwards explain why you rewrote, or
dropped the patch.

> >  b) comment the exact changes needed, or what in general would they do
> 
> ... and told you what was wrong with them or why I think they were not
> necessary.

Mentioning what was wrong after rewriting the code is not the same as comment
on my patch itself.

> >  c) give a chance to the contributor to do the changes themselves
> 
> You complained that the patch submitting process wasted too much time for you
> and that you didn't plan to submit the patches at all and also thought that
> they might not be good for inclusion yet. I assumed that you don't plan to
> improve them and as such I've simply fixed your patches myself (or have done
> them in a way that is acceptable for inclusion)

Well, I did send the patches, didn't I? Maybe I was giving a chance to the
process.

Moreover, that's not an excuse to not follow a process that keeps good
attribution (if you even had such process in place). I would have expected you
to follow this process, and only if I say I'm not interested on doing these
changes myself you go ahead. Just like you should do with everybody else.

In fact, even if you rewrite the patch, in a good review process you would post
the patch here, and only after I had reviewed the patch too, you commit it with
my signed-off-by.

> >  d) leave the original author in the commit
> 
> And if you took at look at the commit message you would see that the rewritten
> change still says that it's based on your patch

"Thanks to Felipe Contreras for the suggestion." Right, that gives a lot of
credit.

That's very far from giving me a chance to be the author of the patch, which in
a way, I am.

Anyway, I have complained about the patch review and contribution process, and
what happened here is a good example of why it doesn't work. All I'm saying is
that FWIW, this was not rewarding, and that's a fact.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.