After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 727878 - qtdemux: replace duplicated variable when parsing trex atom
qtdemux: replace duplicated variable when parsing trex atom
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-09 07:22 UTC by Jimmy Ohn
Modified: 2014-04-10 07:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
remove duplicated code (602 bytes, patch)
2014-04-09 07:22 UTC, Jimmy Ohn
none Details | Review
remove duplicated code (602 bytes, patch)
2014-04-09 07:24 UTC, Jimmy Ohn
needs-work Details | Review
remove duplicagted code and add variable (987 bytes, patch)
2014-04-09 08:18 UTC, Jimmy Ohn
needs-work Details | Review
replace duplicated variable when parsing trex atom (1.33 KB, patch)
2014-04-09 09:25 UTC, Jimmy Ohn
reviewed Details | Review
replace duplicated variable when parsing trex atom (1.33 KB, patch)
2014-04-09 09:39 UTC, Jimmy Ohn
none Details | Review
replace duplicated variable when parsing trex atom (1.33 KB, patch)
2014-04-09 10:33 UTC, Jimmy Ohn
committed Details | Review

Description Jimmy Ohn 2014-04-09 07:22:00 UTC
Created attachment 273853 [details] [review]
remove duplicated code

I found duplicated code in qtdemux_parse_trex function again.
The following patch remove duplicated source code in qtdemux.
Comment 1 Jimmy Ohn 2014-04-09 07:24:17 UTC
Created attachment 273854 [details] [review]
remove duplicated code
Comment 2 Sebastian Dröge (slomo) 2014-04-09 08:01:45 UTC
Comment on attachment 273854 [details] [review]
remove duplicated code

See the comment above the first one. It reads the sample description index, then the second one reads the duration into the same variable and overwrites it.

Maybe change the first one to write to a dummy variable that is not used elsewhere... for clarity. But the code is correct
Comment 3 Jimmy Ohn 2014-04-09 08:15:14 UTC
OK I see.
how about change to following patch?
I thinks that it's better for code readability.


(In reply to comment #2)
> (From update of attachment 273854 [details] [review])
> See the comment above the first one. It reads the sample description index,
> then the second one reads the duration into the same variable and overwrites
> it.
> 
> Maybe change the first one to write to a dummy variable that is not used
> elsewhere... for clarity. But the code is correct
Comment 4 Jimmy Ohn 2014-04-09 08:18:24 UTC
Created attachment 273866 [details] [review]
remove duplicagted code and add variable

How about to change following patch?
I think that it's better for code readability
Comment 5 Sebastian Dröge (slomo) 2014-04-09 08:23:34 UTC
Comment on attachment 273866 [details] [review]
remove duplicagted code and add variable

Yes, can you attach it in "git format-patch" format with your real name and mail address? :)
Comment 6 Jimmy Ohn 2014-04-09 09:24:23 UTC
Ok I upload the patch. is it right?
thank you for your help. :)
Comment 7 Jimmy Ohn 2014-04-09 09:25:06 UTC
Created attachment 273872 [details] [review]
replace duplicated variable when parsing trex atom
Comment 8 Jimmy Ohn 2014-04-09 09:30:48 UTC
(In reply to comment #5)
> (From update of attachment 273866 [details] [review])
> Yes, can you attach it in "git format-patch" format with your real name and
> mail address? :)

(In reply to comment #5)
> (From update of attachment 273866 [details] [review])
> Yes, can you attach it in "git format-patch" format with your real name and
> mail address? :)

Ok I upload the patch. is it right?
thank you for your help. :)
Comment 9 Sebastian Dröge (slomo) 2014-04-09 09:33:14 UTC
Comment on attachment 273872 [details] [review]
replace duplicated variable when parsing trex atom

Could you use your name there instead of having first and last name separated by a .? You can also use non-latin characters for the name btw.
Comment 10 Jimmy Ohn 2014-04-09 09:38:26 UTC
(In reply to comment #9)
> (From update of attachment 273872 [details] [review])
> Could you use your name there instead of having first and last name separated
> by a .? You can also use non-latin characters for the name btw.

I modified it. you are very kind. :)
Comment 11 Jimmy Ohn 2014-04-09 09:39:24 UTC
Created attachment 273873 [details] [review]
replace duplicated variable when parsing trex atom
Comment 12 Jimmy Ohn 2014-04-09 10:32:23 UTC
(In reply to comment #9)
> (From update of attachment 273872 [details] [review])
> Could you use your name there instead of having first and last name separated
> by a .? You can also use non-latin characters for the name btw.

Can I use english name?
I upload patch again as modified my name to english. I'm first time upload source code to open source site. sorry about that. please understand it.
Comment 13 Jimmy Ohn 2014-04-09 10:33:23 UTC
Created attachment 273878 [details] [review]
replace duplicated variable when parsing trex atom
Comment 14 Sebastian Dröge (slomo) 2014-04-10 07:04:16 UTC
Thanks for updating the patch :) Yes, using the latin version of your name is fine too. Just wanted to make sure you know that you can also use non-latin characters there in case you preferred the non-transliterated version of your name :)

commit ecf188e6cd91554ce3a011e37f572ec41909ce18
Author: Jimmy Ohn <yongjin.ohn@lge.com>
Date:   Wed Apr 9 17:30:54 2014 +0900

    qtdemux: replace duplicated variable when parsing trex atom
    
    https://bugzilla.gnome.org/show_bug.cgi?id=727878
Comment 15 Jimmy Ohn 2014-04-10 07:14:50 UTC
thank you very much for your help :)
I found my name below site. wow!
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/