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 757776 - dashdemux: Unit test fails reliably
dashdemux: Unit test fails reliably
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal major
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-08 13:52 UTC by Sebastian Dröge (slomo)
Modified: 2015-11-24 17:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch 1/3 (7.65 KB, patch)
2015-11-09 22:39 UTC, Florin Apostol
committed Details | Review
patch 2/3 (2.72 KB, patch)
2015-11-09 22:39 UTC, Florin Apostol
committed Details | Review
patch 3/3 (1.94 KB, patch)
2015-11-09 22:40 UTC, Florin Apostol
committed Details | Review

Description Sebastian Dröge (slomo) 2015-11-08 13:52:20 UTC
See for example: https://ci.gstreamer.net/job/GStreamer-master/5587/testReport/
Comment 1 Florin Apostol 2015-11-09 13:13:30 UTC
The problems started with commit ccff3be3ab2e5bffcefc12c80a5edb225801f1b9. That commit replaced the HTTPsrc element that adaptivedemux was using with a bin element and made the input generation asynchronous. The tests relied on 2 facts:
1) the input is a fakeHTTPsrc element 
2) the input generation and adaptivedemux output processing is done in the same thread.

To fix the problems we need to:
1) make fakeHTTPsrc thread safe (the configuration functions can now be called from another thread)
2) update tests to correctly reach the fakeHTTPsrc element in the bin so that they can configure the generation of a download error
3) update tests to block the input generation on certain events (so that we can simulate a download error before the asynchronous input generation finishes pushing all data to the queue)

1 and 2 are easy. 3 might be tricky
Comment 2 Thiago Sousa Santos 2015-11-09 20:43:53 UTC
Florin, are you working on this or should I take it?
Comment 3 Florin Apostol 2015-11-09 22:39:36 UTC
Created attachment 315159 [details] [review]
patch 1/3
Comment 4 Florin Apostol 2015-11-09 22:39:57 UTC
Created attachment 315160 [details] [review]
patch 2/3
Comment 5 Florin Apostol 2015-11-09 22:40:20 UTC
Created attachment 315161 [details] [review]
patch 3/3
Comment 6 Florin Apostol 2015-11-09 22:43:47 UTC
Fixed major problems, but there are 2 open issues:
1. we need access to create function from fakeHTTPsrc element. Me and Alex Ashley are working on refactoring the test engine, so testFragmentDownloadError is disabled until that is available.
2. There are some memory leaks in all tests. I've traced the leaks to the newly added bin in adaptive demux, but I cannot figure out who is holding references to this new bin. I will raise a new ticket for that.

With above patches, the test are passing.
Comment 7 Florin Apostol 2015-11-09 23:00:35 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=757859 is tracking the memory leaks
Comment 8 Philippe Normand 2015-11-10 08:15:47 UTC
Thank you, Florin!
Comment 9 Vincent Penquerc'h 2015-11-24 15:49:09 UTC
Review of attachment 315160 [details] [review]:

::: tests/check/elements/dash_demux.c
@@ +1110,3 @@
 
+    /* fakeHTTPsrc element is placed inside a bin. Get the bin */
+    srcBin = gst_object_get_parent (msg->src);

I think this object leaks
Comment 10 Vincent Penquerc'h 2015-11-24 15:57:00 UTC
Nevermind, I just fixed it, it's trivial. I'll push them all once built/tested.
Comment 11 Vincent Penquerc'h 2015-11-24 16:39:40 UTC
commit c6243c7d0085ee3a40f58dfb9d72a1fede6dda21
Author: Florin Apostol <florin.apostol@oregan.net>
Date:   Mon Nov 9 18:07:30 2015 +0000

    adaptivedemux: tests: disabled testFragmentDownloadError test
    
    Until we will have support to control the generating thread from
    fakeHTTPsrc element, the test testFragmentDownloadError is disabled.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757776

commit 665eb6fccae793bd1ed5437b28e0eb8c0aec15aa
Author: Florin Apostol <florin.apostol@oregan.net>
Date:   Mon Nov 9 14:14:34 2015 +0000

    adaptivedemux: tests: corrected access to fakeHTTPsrc element
    
    The src element for adaptivedemux is now a bin. Updated the tests to
    correctly reach into the bin and get the fakeHTTPsrc element
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757776

commit 7f3d47deb3883b3b0d52a6cc739fd5b373767a3b
Author: Florin Apostol <florin.apostol@oregan.net>
Date:   Mon Nov 9 14:13:04 2015 +0000

    adaptivedemux: tests: made fakeHTTPsrc element MT safe
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757776
Comment 12 Florin Apostol 2015-11-24 17:13:12 UTC
(In reply to Vincent Penquerc'h from comment #9)
> Review of attachment 315160 [details] [review] [review]:
> 
> ::: tests/check/elements/dash_demux.c
> @@ +1110,3 @@
>  
> +    /* fakeHTTPsrc element is placed inside a bin. Get the bin */
> +    srcBin = gst_object_get_parent (msg->src);
> 
> I think this object leaks

Thanks for spotting it. I don't know why valgrind didn't pick this.