GStreamer-devel

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

Classic

List

Threaded

5 messages Options

Peter Kjellerstedt

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

Could it be that someone had intended to clamp the value?
I.e., something like this:

update_time = CLAMP (timestamp, mpeg_parse->current_segment.start,
mpeg_parse->current_segment.stop);

I have not looked at the surrounding code so I do not know
if it is the case, but it seems plausible.

//Peter

> -----Original Message-----
> From: Edward Hervey [mailto:[hidden email]]
> Sent: den 21 april 2009 20:41
> To: [hidden email]
> Subject: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead
> assignments.
>
> Module: gst-plugins-ugly
> Branch: master
> Commit: df349f9359d0154ca44bc129e6062a61b68cfba3
> URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-
> ugly/commit/?id=df349f9359d0154ca44bc129e6062a61b68cfba3
>
> Author: Edward Hervey <[hidden email]>
> Date: Tue Apr 21 20:20:02 2009 +0200
>
> mpegstream: Remove dead assignments.
>
> The duplicate assignment of update_time was weird... but it seems
> normal that it's indeed the second statement which is the valid one.
>
> ---
>
> gst/mpegstream/gstmpegdemux.c | 7 ++-----
> gst/mpegstream/gstmpegparse.c | 1 -
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/gst/mpegstream/gstmpegdemux.c
> b/gst/mpegstream/gstmpegdemux.c
> index b867807..c8d55d8 100644
> --- a/gst/mpegstream/gstmpegdemux.c
> +++ b/gst/mpegstream/gstmpegdemux.c
> @@ -1072,7 +1070,6 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux *
> mpeg_demux,
> GstClockTimeDiff diff;
> guint64 update_time;
>
> - update_time = MIN (timestamp, mpeg_parse->current_segment.stop);
> update_time = MAX (timestamp, mpeg_parse->current_segment.start);
> diff = GST_CLOCK_DIFF (mpeg_parse->current_segment.last_stop, update_time);
> if (diff > GST_SECOND * 2) {

------------------------------------------------------------------------------
Stay on top of everything new and different, both inside and
around Java (TM) technology - register by April 22, and save
$200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
300 plus technical and hands-on sessions. Register today.
Use priority code J9JMT32. http://p.sf.net/sfu/p
_______________________________________________
gstreamer-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

Edward Hervey

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

Administrator

On Wed, 2009-04-22 at 11:27 +0200, Peter Kjellerstedt wrote:
> Could it be that someone had intended to clamp the value?
> I.e., something like this:
>
> update_time = CLAMP (timestamp, mpeg_parse->current_segment.start,
> mpeg_parse->current_segment.stop);
>
> I have not looked at the surrounding code so I do not know
> if it is the case, but it seems plausible.

Good point, maybe the maintainers could give use more input (although
I guess it won't matter *that* much considering we're no longer this
demuxer (rank:SECONDARY) but the one in -bad (rank:PRIMARY)).

Edward

P.S. And I'm glad people actually read the commit messages in their
entirety :)

>
> //Peter
>
> > -----Original Message-----
> > From: Edward Hervey [mailto:[hidden email]]
> > Sent: den 21 april 2009 20:41
> > To: [hidden email]
> > Subject: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead
> > assignments.
> >
> > Module: gst-plugins-ugly
> > Branch: master
> > Commit: df349f9359d0154ca44bc129e6062a61b68cfba3
> > URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-
> > ugly/commit/?id=df349f9359d0154ca44bc129e6062a61b68cfba3
> >
> > Author: Edward Hervey <[hidden email]>
> > Date: Tue Apr 21 20:20:02 2009 +0200
> >
> > mpegstream: Remove dead assignments.
> >
> > The duplicate assignment of update_time was weird... but it seems
> > normal that it's indeed the second statement which is the valid one.
> >
> > ---
> >
> > gst/mpegstream/gstmpegdemux.c | 7 ++-----
> > gst/mpegstream/gstmpegparse.c | 1 -
> > 2 files changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/gst/mpegstream/gstmpegdemux.c
> > b/gst/mpegstream/gstmpegdemux.c
> > index b867807..c8d55d8 100644
> > --- a/gst/mpegstream/gstmpegdemux.c
> > +++ b/gst/mpegstream/gstmpegdemux.c
> > @@ -1072,7 +1070,6 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux *
> > mpeg_demux,
> > GstClockTimeDiff diff;
> > guint64 update_time;
> >
> > - update_time = MIN (timestamp, mpeg_parse->current_segment.stop);
> > update_time = MAX (timestamp, mpeg_parse->current_segment.start);
> > diff = GST_CLOCK_DIFF (mpeg_parse->current_segment.last_stop, update_time);
> > if (diff > GST_SECOND * 2) {
>
>
> ------------------------------------------------------------------------------
> Stay on top of everything new and different, both inside and
> around Java (TM) technology - register by April 22, and save
> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
> 300 plus technical and hands-on sessions. Register today.
> Use priority code J9JMT32. http://p.sf.net/sfu/p
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

Wim Taymans

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

Also I'm a bit annoyed with these cleanups. While the compiler has
probably detected something that might not be optimal, it's also
completely irrelevant. All relevant compilers will happily use their
analysis to not compile the dead assignments at all. It's not the job
of the programmer to perform this compiler optimisation step manually,
IMO.

I'm worried that all these cleanups make the code harder to read and
in some cases even remove useful clues to what the author might have
meant in case of a bug (like here?).

Wim

On Wed, Apr 22, 2009 at 12:58 PM, Edward Hervey <[hidden email]> wrote:

> On Wed, 2009-04-22 at 11:27 +0200, Peter Kjellerstedt wrote:
>> Could it be that someone had intended to clamp the value?
>> I.e., something like this:
>>
>> update_time = CLAMP (timestamp, mpeg_parse->current_segment.start,
>> mpeg_parse->current_segment.stop);
>>
>> I have not looked at the surrounding code so I do not know
>> if it is the case, but it seems plausible.
>
> Good point, maybe the maintainers could give use more input (although
> I guess it won't matter *that* much considering we're no longer this
> demuxer (rank:SECONDARY) but the one in -bad (rank:PRIMARY)).
>
> Edward
>
> P.S. And I'm glad people actually read the commit messages in their
> entirety :)
>
>>
>> //Peter
>>
>> > -----Original Message-----
>> > From: Edward Hervey [mailto:[hidden email]]
>> > Sent: den 21 april 2009 20:41
>> > To: [hidden email]
>> > Subject: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead
>> > assignments.
>> >
>> > Module: gst-plugins-ugly
>> > Branch: master
>> > Commit: df349f9359d0154ca44bc129e6062a61b68cfba3
>> > URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-
>> > ugly/commit/?id=df349f9359d0154ca44bc129e6062a61b68cfba3
>> >
>> > Author: Edward Hervey <[hidden email]>
>> > Date: Tue Apr 21 20:20:02 2009 +0200
>> >
>> > mpegstream: Remove dead assignments.
>> >
>> > The duplicate assignment of update_time was weird... but it seems
>> > normal that it's indeed the second statement which is the valid one.
>> >
>> > ---
>> >
>> > gst/mpegstream/gstmpegdemux.c | 7 ++-----
>> > gst/mpegstream/gstmpegparse.c | 1 -
>> > 2 files changed, 2 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/gst/mpegstream/gstmpegdemux.c
>> > b/gst/mpegstream/gstmpegdemux.c
>> > index b867807..c8d55d8 100644
>> > --- a/gst/mpegstream/gstmpegdemux.c
>> > +++ b/gst/mpegstream/gstmpegdemux.c
>> > @@ -1072,7 +1070,6 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux *
>> > mpeg_demux,
>> > GstClockTimeDiff diff;
>> > guint64 update_time;
>> >
>> > - update_time = MIN (timestamp, mpeg_parse->current_segment.stop);
>> > update_time = MAX (timestamp, mpeg_parse->current_segment.start);
>> > diff = GST_CLOCK_DIFF (mpeg_parse->current_segment.last_stop, update_time);
>> > if (diff > GST_SECOND * 2) {
>>
>>
>> ------------------------------------------------------------------------------
>> Stay on top of everything new and different, both inside and
>> around Java (TM) technology - register by April 22, and save
>> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
>> 300 plus technical and hands-on sessions. Register today.
>> Use priority code J9JMT32. http://p.sf.net/sfu/p
>> _______________________________________________
>> gstreamer-devel mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
>
>
> ------------------------------------------------------------------------------
> Stay on top of everything new and different, both inside and
> around Java (TM) technology - register by April 22, and save
> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
> 300 plus technical and hands-on sessions. Register today.
> Use priority code J9JMT32. http://p.sf.net/sfu/p
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
>

Edward Hervey

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

Administrator

On Wed, 2009-04-22 at 14:36 +0200, Wim Taymans wrote:
> Also I'm a bit annoyed with these cleanups. While the compiler has
> probably detected something that might not be optimal, it's also
> completely irrelevant. All relevant compilers will happily use their
> analysis to not compile the dead assignments at all. It's not the job
> of the programmer to perform this compiler optimisation step manually,
> IMO.

gcc doesn't do it when I checked a few weeks back, unless you don't
consider it a relevent compiler.

>
> I'm worried that all these cleanups make the code harder to read and
> in some cases even remove useful clues to what the author might have
> meant in case of a bug (like here?).

I'm not removing those (there's a ton of dead assignments in various
demuxer's parsing code which I didn't/won't commit).

What I'll do from now on for those is put big fat FIXME warnings
before each of those. Like that it at least won't go unnoticed and won't
bother people (apparently?).

Edward

>
> Wim
>
> On Wed, Apr 22, 2009 at 12:58 PM, Edward Hervey <[hidden email]> wrote:
> > On Wed, 2009-04-22 at 11:27 +0200, Peter Kjellerstedt wrote:
> >> Could it be that someone had intended to clamp the value?
> >> I.e., something like this:
> >>
> >> update_time = CLAMP (timestamp, mpeg_parse->current_segment.start,
> >> mpeg_parse->current_segment.stop);
> >>
> >> I have not looked at the surrounding code so I do not know
> >> if it is the case, but it seems plausible.
> >
> > Good point, maybe the maintainers could give use more input (although
> > I guess it won't matter *that* much considering we're no longer this
> > demuxer (rank:SECONDARY) but the one in -bad (rank:PRIMARY)).
> >
> > Edward
> >
> > P.S. And I'm glad people actually read the commit messages in their
> > entirety :)
> >
> >>
> >> //Peter
> >>
> >> > -----Original Message-----
> >> > From: Edward Hervey [mailto:[hidden email]]
> >> > Sent: den 21 april 2009 20:41
> >> > To: [hidden email]
> >> > Subject: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead
> >> > assignments.
> >> >
> >> > Module: gst-plugins-ugly
> >> > Branch: master
> >> > Commit: df349f9359d0154ca44bc129e6062a61b68cfba3
> >> > URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-
> >> > ugly/commit/?id=df349f9359d0154ca44bc129e6062a61b68cfba3
> >> >
> >> > Author: Edward Hervey <[hidden email]>
> >> > Date: Tue Apr 21 20:20:02 2009 +0200
> >> >
> >> > mpegstream: Remove dead assignments.
> >> >
> >> > The duplicate assignment of update_time was weird... but it seems
> >> > normal that it's indeed the second statement which is the valid one.
> >> >
> >> > ---
> >> >
> >> > gst/mpegstream/gstmpegdemux.c | 7 ++-----
> >> > gst/mpegstream/gstmpegparse.c | 1 -
> >> > 2 files changed, 2 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/gst/mpegstream/gstmpegdemux.c
> >> > b/gst/mpegstream/gstmpegdemux.c
> >> > index b867807..c8d55d8 100644
> >> > --- a/gst/mpegstream/gstmpegdemux.c
> >> > +++ b/gst/mpegstream/gstmpegdemux.c
> >> > @@ -1072,7 +1070,6 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux *
> >> > mpeg_demux,
> >> > GstClockTimeDiff diff;
> >> > guint64 update_time;
> >> >
> >> > - update_time = MIN (timestamp, mpeg_parse->current_segment.stop);
> >> > update_time = MAX (timestamp, mpeg_parse->current_segment.start);
> >> > diff = GST_CLOCK_DIFF (mpeg_parse->current_segment.last_stop, update_time);
> >> > if (diff > GST_SECOND * 2) {
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Stay on top of everything new and different, both inside and
> >> around Java (TM) technology - register by April 22, and save
> >> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
> >> 300 plus technical and hands-on sessions. Register today.
> >> Use priority code J9JMT32. http://p.sf.net/sfu/p
> >> _______________________________________________
> >> gstreamer-devel mailing list
> >> [hidden email]
> >> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
> >
> >
> > ------------------------------------------------------------------------------
> > Stay on top of everything new and different, both inside and
> > around Java (TM) technology - register by April 22, and save
> > $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
> > 300 plus technical and hands-on sessions. Register today.
> > Use priority code J9JMT32. http://p.sf.net/sfu/p
> > _______________________________________________
> > gstreamer-devel mailing list
> > [hidden email]
> > https://lists.sourceforge.net/lists/listinfo/gstreamer-devel
> >
>
> ------------------------------------------------------------------------------
> Stay on top of everything new and different, both inside and
> around Java (TM) technology - register by April 22, and save
> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
> 300 plus technical and hands-on sessions. Register today.
> Use priority code J9JMT32. http://p.sf.net/sfu/p
> _______________________________________________
> gstreamer-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel

Stefan Sauer

Re: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead assignments.

In reply to this post by Wim Taymans

Wim Taymans schrieb:

> Also I'm a bit annoyed with these cleanups. While the compiler has
> probably detected something that might not be optimal, it's also
> completely irrelevant. All relevant compilers will happily use their
> analysis to not compile the dead assignments at all. It's not the job
> of the programmer to perform this compiler optimisation step manually,
> IMO.
>
> I'm worried that all these cleanups make the code harder to read and
> in some cases even remove useful clues to what the author might have
> meant in case of a bug (like here?).
>
> Wim

I think Edward did a good job here. Compilers do indeed leave a lot of dead code
in. E.g. removing dummy _init() or _set/_get_property functions is nice, as it
makes the code smaller. I think we have enough plugin to copy'n'paste
boilerplate when we atuall need it.

One controversical thing is to remove dead assignments, like "ret=FALSE in some
branches" and dead assignments in return values. There was one change in core,
where it might have been better to turn the dead assignment of the return result
into an if + GST_WARNING.

Stefan

>
> On Wed, Apr 22, 2009 at 12:58 PM, Edward Hervey <[hidden email]> wrote:
>> On Wed, 2009-04-22 at 11:27 +0200, Peter Kjellerstedt wrote:
>>> Could it be that someone had intended to clamp the value?
>>> I.e., something like this:
>>>
>>> update_time = CLAMP (timestamp, mpeg_parse->current_segment.start,
>>> mpeg_parse->current_segment.stop);
>>>
>>> I have not looked at the surrounding code so I do not know
>>> if it is the case, but it seems plausible.
>> Good point, maybe the maintainers could give use more input (although
>> I guess it won't matter *that* much considering we're no longer this
>> demuxer (rank:SECONDARY) but the one in -bad (rank:PRIMARY)).
>>
>> Edward
>>
>> P.S. And I'm glad people actually read the commit messages in their
>> entirety :)
>>
>>> //Peter
>>>
>>>> -----Original Message-----
>>>> From: Edward Hervey [mailto:[hidden email]]
>>>> Sent: den 21 april 2009 20:41
>>>> To: [hidden email]
>>>> Subject: [gst-cvs] gst-plugins-ugly: mpegstream: Remove dead
>>>> assignments.
>>>>
>>>> Module: gst-plugins-ugly
>>>> Branch: master
>>>> Commit: df349f9359d0154ca44bc129e6062a61b68cfba3
>>>> URL: http://cgit.freedesktop.org/gstreamer/gst-plugins-
>>>> ugly/commit/?id=df349f9359d0154ca44bc129e6062a61b68cfba3
>>>>
>>>> Author: Edward Hervey <[hidden email]>
>>>> Date: Tue Apr 21 20:20:02 2009 +0200
>>>>
>>>> mpegstream: Remove dead assignments.
>>>>
>>>> The duplicate assignment of update_time was weird... but it seems
>>>> normal that it's indeed the second statement which is the valid one.
>>>>
>>>> ---
>>>>
>>>> gst/mpegstream/gstmpegdemux.c | 7 ++-----
>>>> gst/mpegstream/gstmpegparse.c | 1 -
>>>> 2 files changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/gst/mpegstream/gstmpegdemux.c
>>>> b/gst/mpegstream/gstmpegdemux.c
>>>> index b867807..c8d55d8 100644
>>>> --- a/gst/mpegstream/gstmpegdemux.c
>>>> +++ b/gst/mpegstream/gstmpegdemux.c
>>>> @@ -1072,7 +1070,6 @@ gst_mpeg_demux_send_subbuffer (GstMPEGDemux *
>>>> mpeg_demux,
>>>> GstClockTimeDiff diff;
>>>> guint64 update_time;
>>>>
>>>> - update_time = MIN (timestamp, mpeg_parse->current_segment.stop);
>>>> update_time = MAX (timestamp, mpeg_parse->current_segment.start);
>>>> diff = GST_CLOCK_DIFF (mpeg_parse->current_segment.last_stop, update_time);
>>>> if (diff > GST_SECOND * 2) {
>>>
>>> ------------------------------------------------------------------------------
>>> Stay on top of everything new and different, both inside and
>>> around Java (TM) technology - register by April 22, and save
>>> $200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
>>> 300 plus technical and hands-on sessions. Register today.
>>> Use priority code J9JMT32. http://p.sf.net/sfu/p
>>> _______________________________________________
>>> gstreamer-devel mailing list
>>> [hidden email]
>>> https://lists.sourceforge.net/lists/listinfo/gstreamer-devel