GNOME Bugzilla – Bug 758384
dashdemux: tests: refactor into adaptive_engine components
Last modified: 2015-12-22 15:57:12 UTC
Florin and I have been working on refactoring of his dashdemux tests so that they can be re-used by other elements based upon GstAdaptiveDemux. This has allowed me to create a set of tests for hlsdemux. I am going to attach patches to this ticket for the refactoring and some hlsdemux tests. As part of this work I found a couple of bugs, but I am going to submit those tests (and bug fixes) as separate tickets.
Created attachment 315946 [details] [review] dashdemux: tests: Refactor into adaptive_engine components To allow code from dash_demux.c to be used by other elements that are based upon GstAdaptiveDemux, the code has been refactored into four new files: adaptive_demux_engine.[ch] adaptive_demux_common.[ch] The code in adaptive_demux_engine.c provides a generic test engine for elements based upon GstAdaptiveDemux. The code in adaptive_demux_common.c provides a set of utility functions that are common between the tests for hlsdemux and dashdemux. This patch is based upon the head of git master with the three patches from bug 757776 [1] applied. [1] https://bugzilla.gnome.org/show_bug.cgi?id=757776
Created attachment 315947 [details] [review] hlsdemux: tests: add unit tests for hlsdemux
Created attachment 315948 [details] [review] Using the new GstAdaptiveDemux test framework, add tests that exercise hlsdemux additional test for hlsdemux_m3u8
Created attachment 315949 [details] hlsdemux: tests: check URL joining if media URL contains a '/' character additional test for hlsdemux_m3u8
Created attachment 315950 [details] hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag additional test for hlsdemux_m3u8
Created attachment 315952 [details] [review] hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag
Created attachment 315953 [details] [review] hlsdemux: tests: check URL joining if media URL contains a '/' character
To Vincent (or any other maintainer): can you please merge these changes and their dependencies? We need them for writing new dash and hls tests. They will also fix the broken dash tests. Thank you.
Review of attachment 315946 [details] [review]: ::: tests/check/elements/adaptive_demux_common.c @@ +171,3 @@ + if (testOutputStreamData->expected_data) { + gsize size = gst_buffer_get_size (buffer); + fail_unless ((streamOffset + size) <= testOutputStreamData->expected_size); Not sure I understand this. What happens if you would seek to byte=100 and was only expecting to receive 10 bytes after it. What would expected_size contain in this case and how streamOffset + size can be smaller than the expected size?
Review of attachment 315946 [details] [review]: ::: tests/check/elements/dash_demux.c @@ +26,3 @@ +#define COPY_OUTPUT_TEST_DATA(outputTestData,testData) do { \ + guint otdLen = sizeof((outputTestData)) / sizeof((outputTestData)[0]); \ + for(guint otdPos=0; otdPos<otdLen; ++otdPos){ \ Gstreamer standard is to declare loop counters out of the loop @@ +29,3 @@ + (testData)->output_streams = g_list_append ((testData)->output_streams, &(outputTestData)[otdPos]); \ + } \ + } while(0); Remove the ; so we don't accidentally get two statements @@ +257,3 @@ " </Representation></AdaptationSet></Period></MPD>"; + GstDashDemuxTestInputData inputTestData[] = { Why is const removed ? Is it modified now ? Should also be static. Same for other test data below.
(In reply to Thiago Sousa Santos from comment #9) > Review of attachment 315946 [details] [review] [review]: > > ::: tests/check/elements/adaptive_demux_common.c > @@ +171,3 @@ > + if (testOutputStreamData->expected_data) { > + gsize size = gst_buffer_get_size (buffer); > + fail_unless ((streamOffset + size) <= > testOutputStreamData->expected_size); > > Not sure I understand this. > > What happens if you would seek to byte=100 and was only expecting to receive > 10 bytes after it. > > What would expected_size contain in this case and how streamOffset + size > can be smaller than the expected size? The seek function currently seeks only at the beginning of the file. The seek test waits for some data to be sent, then generates a seek to the beginning of the file. The seek request will make adaptive demux push a new segment event downstream. The test detects it and in on_demuxReceivesEvent function adds stream->segment_received_size to total_received_size and resets segment_received_size to 0. It also sets segment_start to segment->start, so I think seeks to the middle of the file will also work OK. I did not test them yet. expected_size is the size of expected_buffer (which is defined and filled by the test). Only hls tests are using expected_buffer. The dash tests use a pattern for the generated data in order to check that the correct data is received. expected_size might be a confusing name (you thought is the number of bytes we expect to receive, but it's not. It's the size of expected_data buffer). The fail_if is an assert so that memcmp below will not make an illegal memory access. The test needs to define an expected_buffer big enough to contain the data corresponding to all the positions that could be played. For example, if you want to play the first 10s and then seek forward to position 1 minute, I think the expected buffer should contain the data for the whole minute, even if some of it will not be checked.
(In reply to Florin Apostol from comment #11) > (In reply to Thiago Sousa Santos from comment #9) > > Review of attachment 315946 [details] [review] [review] [review]: > > > > ::: tests/check/elements/adaptive_demux_common.c > > @@ +171,3 @@ > > + if (testOutputStreamData->expected_data) { > > + gsize size = gst_buffer_get_size (buffer); > > + fail_unless ((streamOffset + size) <= > > testOutputStreamData->expected_size); > > > > Not sure I understand this. > > > > What happens if you would seek to byte=100 and was only expecting to receive > > 10 bytes after it. > > > > What would expected_size contain in this case and how streamOffset + size > > can be smaller than the expected size? > > The seek function currently seeks only at the beginning of the file. The > seek test waits for some data to be sent, then generates a seek to the > beginning of the file. > The seek request will make adaptive demux push a new segment event > downstream. The test detects it and in on_demuxReceivesEvent function adds > stream->segment_received_size to total_received_size and resets > segment_received_size to 0. It also sets segment_start to segment->start, so > I think seeks to the middle of the file will also work OK. I did not test > them yet. > > expected_size is the size of expected_buffer (which is defined and filled by > the test). Only hls tests are using expected_buffer. The dash tests use a > pattern for the generated data in order to check that the correct data is > received. > > expected_size might be a confusing name (you thought is the number of bytes > we expect to receive, but it's not. It's the size of expected_data buffer). > > The fail_if is an assert so that memcmp below will not make an illegal > memory access. The test needs to define an expected_buffer big enough to > contain the data corresponding to all the positions that could be played. > For example, if you want to play the first 10s and then seek forward to > position 1 minute, I think the expected buffer should contain the data for > the whole minute, even if some of it will not be checked. Disregard the above comment, its wrong. expected_size is indeed the size expected to be received.
Review of attachment 315953 [details] [review]: ::: tests/check/elements/hlsdemux_m3u8.c @@ +1338,3 @@ + assert_equals_string (media->uri, + "http://localhost/1251/media.m3u8?acl=/*1054559_h264_1500k.mp4"); + g_print ("%s\n", media->uri); This trace is problably meant to be removed
Review of attachment 315947 [details] [review]: ::: tests/check/elements/hls_demux.c @@ +63,3 @@ + } + memset (mpeg_ts->data, 0, length); + for (pos = 0; pos < length; pos += 188) { This seems a bit dangerous since it only stays within bounds if length%188==0
(In reply to Thiago Sousa Santos from comment #9) > Review of attachment 315946 [details] [review] [review]: > > ::: tests/check/elements/adaptive_demux_common.c > @@ +171,3 @@ > + if (testOutputStreamData->expected_data) { > + gsize size = gst_buffer_get_size (buffer); > + fail_unless ((streamOffset + size) <= > testOutputStreamData->expected_size); > > Not sure I understand this. > > What happens if you would seek to byte=100 and was only expecting to receive > 10 bytes after it. > > What would expected_size contain in this case and how streamOffset + size > can be smaller than the expected size? This fail_unless is wrong and should be removed. When I added it I thought expected_size will be the size of expected_data (which is correct for many situations, but not all of them) and I wanted to protect the memcp.
Created attachment 316542 [details] [review] dashdemux: tests: Refactor into adaptive_engine components
Created attachment 316543 [details] [review] hlsdemux: tests: add unit tests for hlsdemux
Created attachment 316544 [details] [review] hlsdemux: tests: check URL joining if media URL contains a '/' character
Review of attachment 316542 [details] [review]: ::: tests/check/elements/dash_demux.c @@ +142,3 @@ " </SegmentBase>" " </Representation></AdaptationSet></Period></MPD>"; + GstDashDemuxTestInputData inputTestData[] = { you forgot a const here
Thiago, are you OK with these now, wrt your comment #9 ?
Review of attachment 315952 [details] [review]: ::: tests/check/elements/hlsdemux_m3u8.c @@ +1340,3 @@ + assert_equals_int64 (media->height, 352); + assert_equals_int64 (media->bandwidth, 1251135); + assert_equals_string (media->codecs, "\"avc1.42001f, mp4a.40.2\""); Do we need to keep the ""? Are they always present or are they useful for some case?
(In reply to Vincent Penquerc'h from comment #20) > Thiago, are you OK with these now, wrt your comment #9 ? Looks good to me, except the const thing already mentioned in comment #19.
(In reply to Thiago Sousa Santos from comment #21) > Review of attachment 315952 [details] [review] [review]: > > ::: tests/check/elements/hlsdemux_m3u8.c > @@ +1340,3 @@ > + assert_equals_int64 (media->height, 352); > + assert_equals_int64 (media->bandwidth, 1251135); > + assert_equals_string (media->codecs, "\"avc1.42001f, mp4a.40.2\""); > > Do we need to keep the ""? Are they always present or are they useful for > some case? I think they should be deleted, I was being lazy and avoiding touching code in m3u8.c.
Created attachment 317117 [details] [review] hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag Add a test to check that parsing of the fields in the EXT-X-STREAM tag is correct. The codecs parameter was incorrectly including the quote characters at each end of the string. This patch removes these quotes and updates the existing test cases to this new behaviour.
Created attachment 317118 [details] [review] dashdemux: tests: Refactor into adaptive_engine components
(In reply to Florin Apostol from comment #19) > Review of attachment 316542 [details] [review] [review]: > > ::: tests/check/elements/dash_demux.c > @@ +142,3 @@ > " </SegmentBase>" > " </Representation></AdaptationSet></Period></MPD>"; > + GstDashDemuxTestInputData inputTestData[] = { > > you forgot a const here Actually the input data cannot be const, because the user_data parameter to the gst_test_http_src_install_callbacks function is not const. In the case of some of the hlsdemux tests, the contents of the user_data is modified by the tests.
(In reply to A Ashley from comment #26) > > Actually the input data cannot be const, because the user_data parameter to > the gst_test_http_src_install_callbacks function is not const. In the case > of some of the hlsdemux tests, the contents of the user_data is modified by > the tests. That logic is not correct. Even if user_data is not const, input data can be const. user_data is a pointer to a structure GstDashDemuxTestCase. One of the structure members is pointer to const (const GstDashDemuxTestInputData *input;). That member can be initialized with another pointer to const (the input data).
> > That logic is not correct. Even if user_data is not const, input data can be > const. user_data is a pointer to a structure GstDashDemuxTestCase. One of > the structure members is pointer to const (const GstDashDemuxTestInputData > *input;). That member can be initialized with another pointer to const (the > input data). Check the patch, the inputTestData is passed directly to gst_test_http_src_install_callbacks. You are thinking of a later patch where you wrapped GstDashDemuxTestInputData inside a GstDashDemuxTestCase.
(In reply to A Ashley from comment #28) > Check the patch, the inputTestData is passed directly to > gst_test_http_src_install_callbacks. > > You are thinking of a later patch where you wrapped > GstDashDemuxTestInputData inside a GstDashDemuxTestCase. You are right. Too many patches to keep track of their individual changes!
So all comments appear addressed, and the current patches pass make check here. If there's no other comment, I will push those changes to master later today.
Comment on attachment 317117 [details] [review] hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag >hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag You are changing the behaviour of code here in a commit that says "add test", which seems suboptimal to me (although you do mention it of course). >@@ -234,6 +234,16 @@ parse_attributes (gchar ** ptr, gchar ** a, gchar ** v) > *v = p = g_utf8_strchr (*ptr, -1, '='); > if (*v) { > *v = g_utf8_next_char (*v); >+ if (**v == '"') { >+ ve = g_utf8_next_char (*v); >+ if (ve) { >+ ve = g_utf8_strchr (ve, -1, '"'); >+ if (ve) { >+ *v = g_utf8_next_char (*v); >+ *ve = '\0'; >+ } >+ } >+ } > *p = '\0'; > } else { So we just unconditionally unquote all values now?
(In reply to Tim-Philipp Müller from comment #31) > Comment on attachment 317117 [details] [review] [review] > hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag > > >hlsdemux: tests: add test of parsing the EXT-X-STREAM-INF tag > > You are changing the behaviour of code here in a commit that says "add > test", which seems suboptimal to me (although you do mention it of course). > Yes, I think the commit message should be the other way around: 1. state that the commit causes quoted-string attributes to be unquoted 2. and describe that the unit tests have been updated to the new behaviour. > So we just unconditionally unquote all values now? Yes. The two other places that use quoted strings are for parsing the URI of key files and index files. In both those cases they are unconditionally unquoted. We can now delete the unquote_string() function in m3u8.c because it happens during m3u8 parsing.
Created attachment 317216 [details] [review] hlsdemux: unquote all the quoted-string attributes The URI attribute from the EXT-X-KEY tag and the URI attribute from the EXT-X-I-FRAMES-ONLY tag are both quoted-string attibutes that have their quotation marks removed during parsing. The CODECS attribute of the EXT-X-STREAM-INF is also a quoted-string attribute, but this attribute was not being un-quoted. This patch changes the parser to always unquote all quoted-string attributes and adjusts the unit tests to this new bevahiour for the CODECS attribute. An additional test is added to check that parsing of all of the fields in the EXT-X-STREAM tag is correct, including those that contain comma characters.
Created attachment 317217 [details] [review] hlsdemux: unquote all the quoted-string attributes Doh! Uploaded wrong version. Sorry, here's the correct version. The difference between the two versions is that the previous version failed to produce a GST_WARNING for a quoted-string that was missing the final quote mark.
One minor thing is the check for state change from playing to paused being a bit mysterious on the seek handling but it works. Perhaps adding a comment would make it clear that it is one of the signals the seek event was fully handled? Any other ideas on how to improve that part and make it a bit more clear? Anyway it doesn't justify holding these patches in bugzilla for longer. Thanks for the changes. Forgot to ensure the bug url on the bottom commits, but here they are: commit ebf6de33d2744fd68f6273e6f66dce94b8c3f552 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Tue Nov 10 16:25:53 2015 +0000 hlsdemux: tests: check URL joining if media URL contains a '/' character If the query parameter (for example http://example.net/1054559_1500k.mp4/master.m3u8?acl=/*1054559_1500k.mp4), check that m3u8.c correctly converts the relative URLs of the media playlists in to absolute URLs. It must not use the last '/' it finds in the URL, as according to RFC3986 the '/' character is allowed in the query part of the URL. https://bugzilla.gnome.org/show_bug.cgi?id=758384 commit f6bff8f5f5ca14cae5774d79e7b7c4ac0542a67f Author: Alex Ashley <bugzilla@ashley-family.net> Date: Tue Nov 10 16:23:59 2015 +0000 hlsdemux: unquote all the quoted-string attributes The URI attribute from the EXT-X-KEY tag and the URI attribute from the EXT-X-I-FRAMES-ONLY tag are both quoted-string attibutes that have their quotation marks removed during parsing. The CODECS attribute of the EXT-X-STREAM-INF is also a quoted-string attribute, but this attribute was not being un-quoted. This commit changes the parser to always unquote all quoted-string attributes and adjusts the unit tests to this new bevahiour for the CODECS attribute. An additional test is added to check that parsing of all of the fields in the EXT-X-STREAM tag is correct, including those that contain comma characters. https://bugzilla.gnome.org/show_bug.cgi?id=758384 commit eafdf5673a38537fc41cfdc08c29321810387a39 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Tue Nov 10 16:41:02 2015 +0000 hlsdemux: tests: add unit tests for hlsdemux Using the new GstAdaptiveDemux test framework, add tests that exercise hlsdemux. The following tests are added: simpleTest A simple playlist that contains some media URLs testMediaPlaylist A master playlist with a variant playlist that contains media URLs testMediaPlaylistNotFound A master playlist that points to a missing variant playlist testFragmentNotFound A master playlist with a variant playlist that contains media URLs There is a missing media file referenced from the variant playlist. testFragmentDownloadError A master playlist with a variant playlist that contains media URLs During the download of one media file, the test simulates the network connection being dropped. testSeek A simple test of trying to perform a seek on an HLS stream. commit ae3ed25025e34ea9b09df59d22d7ebd7294560bc Author: Alex Ashley <bugzilla@ashley-family.net> Date: Tue Nov 10 13:13:35 2015 +0000 dashdemux: tests: Refactor into adaptive_engine components To allow code from dash_demux.c to be used by other elements that are based upon GstAdaptiveDemux, the code has been refactored into four new files: adaptive_demux_engine.[ch] adaptive_demux_common.[ch] The code in adaptive_demux_engine.c provides a generic test engine for elements based upon GstAdaptiveDemux. The code in adaptive_demux_common.c provides a set of utility functions that are common between the tests for hlsdemux and dashdemux. As part of the refactoring, variables in structures were renamed from using camelCase to underscore_case to match other GStreamer source code. The fake_http_src was renamed test_http_src and changed to use callbacks to provide input data and error conditions. Rather than using an array of input data that tries to encode all the possible use cases for the GstTestHTTPSrc element, use a struct of callbacks. Users of this element are obliged to implement at least the src_start callback, which provides a way to link from a URI to the settings for that URI.
Thanks Thiago. That should make Florin's life a bit easier with his work on testing the DASH clock drift code. The previous test structure did not support dynamic creation of HTTP responses.