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 794902 - qtdemux tests: Add edit list test battery
qtdemux tests: Add edit list test battery
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-02 15:42 UTC by Alicia Boya García
Modified: 2018-11-03 15:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux tests: Add edit list test battery (64.73 KB, patch)
2018-04-02 15:42 UTC, Alicia Boya García
none Details | Review
qtdemux tests: Add edit list test battery (64.83 KB, patch)
2018-04-02 21:55 UTC, Alicia Boya García
none Details | Review
qtdemux tests: Add edit list test battery (64.93 KB, patch)
2018-04-04 12:06 UTC, Alicia Boya García
none Details | Review

Description Alicia Boya García 2018-04-02 15:42:37 UTC
Edit list handling has enough complexity to warrant some tests.

This patch introduces a test battery that generates a variety of edit
lists in different types of files (fragmented or not fragmented) and
tests them in both push and pull mode.

Since the valid combinations of all the test variables are quite a few
(35 tests), code generation is used for the actual test functions.
The code generator also defines the expected failures due to bugs not
currently fixed.
Comment 1 Alicia Boya García 2018-04-02 15:42:50 UTC
Created attachment 370443 [details] [review]
qtdemux tests: Add edit list test battery
Comment 2 Alicia Boya García 2018-04-02 16:32:36 UTC
The attached tests pass (sans expected failures) after the following patches for bugs in qtdemux:

1. qtdemux: Properly handle edit list in push mode
https://bugzilla.gnome.org/show_bug.cgi?id=778426
https://bug778426.bugzilla-attachments.gnome.org/attachment.cgi?id=370372

2. qtdemux: edits with duration = 0 don't work on files without a mehd
https://bugzilla.gnome.org/show_bug.cgi?id=794858
https://bug794858.bugzilla-attachments.gnome.org/attachment.cgi?id=370389

3. (Not before #1, preferrably also not before #2) qtdemux: add support for edit lists for fragmented files in push mode
https://bugzilla.gnome.org/show_bug.cgi?id=793058
https://bug793058.bugzilla-attachments.gnome.org/attachment.cgi?id=370391

4. qtdemux: Premature EOS when some files are played in push mode
https://bugzilla.gnome.org/show_bug.cgi?id=794199
https://bug794199.bugzilla-attachments.gnome.org/attachment.cgi?id=370448


And now that we are at it, some related code style patches would be nice too:

5. (Code clarity) qtdemux: Clarify variable name
https://bugzilla.gnome.org/show_bug.cgi?id=794903
https://bug794903.bugzilla-attachments.gnome.org/attachment.cgi?id=370447

6. (Code clarity) qtdemux_parse_segments: remove superfluous variable
https://bugzilla.gnome.org/show_bug.cgi?id=793751
https://bug793751.bugzilla-attachments.gnome.org/attachment.cgi?id=368824
Comment 3 Sebastian Dröge (slomo) 2018-04-02 17:16:39 UTC
Thanks! I'm not sure the dependency on node will be generally liked, but having tests for this is certainly a good idea.
Comment 4 Alicia Boya García 2018-04-02 17:24:22 UTC
I've made a GitHub branch with all these patches together:

https://github.com/ntrrgc/gst-plugins-good/tree/20180402-edit-lists-tests-squash
Comment 5 Nicolas Dufresne (ndufresne) 2018-04-02 18:52:17 UTC
Review of attachment 370443 [details] [review]:

I don't like the Javascript generator, worst it generate a .cpp that get included in a C file. I think all this can be done with macro.
Comment 6 Alicia Boya García 2018-04-02 21:38:43 UTC
(In reply to Nicolas Dufresne (stormer) from comment #5)
> Review of attachment 370443 [details] [review] [review]:
> 
> I don't like the Javascript generator, worst it generate a .cpp that get
> included in a C file.

The .cpp extension is just a typo.

> I think all this can be done with macro.

I prefer JS over macros for this because it has features like:

* Conditionals
* Loops
* Lists
* Template strings
* Local variables
* Strings functions (e.g. toUpperCase)
* Procedures (functions that mutate the state of variables)
* Ability to generate newlines in the generated code

All of which C preprocessor, as a language, lacks. If you think you can replicate my code generator with clean C preprocessor code, you are welcome to do so; I don't know how to.

Bear in mind this is not only a triple foreach with the variable combinations, but filtering those that are not supported and those that are known to trigger expected bugs.

The options here are:

a) Use the JS generator. If you are not working on the tests, you don't have to do anything because the generated code is already included for this purpose. If one day you want to disable some tests or add new combinations, you would do so in the JS file and run it with node. (You could even paste it in the console of your browser instead if you don't even want to install node).

b) Rewrite the JS generator in another language you may prefer, e.g. Python, which has worse template strings (they require Py3.6 and cannot be used recursively or with complex expressions) but would work exactly in the same way. Still, compared with C macro language it is heaven on Earth and could even be made into an automatic build target in meson if desired without adding more dependencies. [In this case, it should be clarified what Python version to use.]

c) Spend hours researching arcane C macro tricks to bring a monster to life that somehow performs the same work inside the C compiler. I tried just for long enough to become aware of how a bad idea that is.

d) No code generation. Easy, just paste the generated code in the test file and let it be. The result would be big error-prone tables I would not dare to put my bare hands on. Commenting out a single test in this way is easy, so it may be tempting; but this way there is no concise way to explain *why* those tests are disabled or buggy (a known failure may often have several bugs related) or toggling those conditions when working on the bugs.

e) No tests at all. The easiest. No way to detect regression on time though...

Before going to any of these ways it may be a good idea to ask: is a small, self-contained JavaScript generator really such a big problem -- especially considering the tradeoffs we are making?

What option do you prefer?
Comment 7 Alicia Boya García 2018-04-02 21:55:51 UTC
Created attachment 370458 [details] [review]
qtdemux tests: Add edit list test battery

Edit list handling has enough complexity to warrant some tests.

This patch introduces a test battery that generates a variety of edit
lists in different types of files (fragmented or not fragmented) and
tests them in both push and pull mode.

Since the valid combinations of all the test variables are quite a few
(35 tests), code generation is used for the actual test functions.
The code generator also defines the expected failures due to bugs not
currently fixed.
Comment 8 Nicolas Dufresne (ndufresne) 2018-04-02 22:21:33 UTC
I'm sorry, but I cannot decide for yourself, even if you write the Bible about it. I'll let others put their opinion about this.
Comment 9 Sebastian Dröge (slomo) 2018-04-03 06:47:21 UTC
I'm ok with this as long as there's documentation how to run this and what has to be installed, e.g. at the top of the file. This only has to be run when changing the tests after all.
Comment 10 Alicia Boya García 2018-04-04 12:04:26 UTC
(In reply to Sebastian Dröge (slomo) from comment #9)
> I'm ok with this as long as there's documentation how to run this and what
> has to be installed, e.g. at the top of the file. This only has to be run
> when changing the tests after all.

This is already the case, but I'll modify it slightly to make it more explicit just in case.
Comment 11 Alicia Boya García 2018-04-04 12:06:34 UTC
Created attachment 370524 [details] [review]
qtdemux tests: Add edit list test battery

Edit list handling has enough complexity to warrant some tests.

This patch introduces a test battery that generates a variety of edit
lists in different types of files (fragmented or not fragmented) and
tests them in both push and pull mode.

Since the valid combinations of all the test variables are quite a few
(35 tests), code generation is used for the actual test functions.
The code generator also defines the expected failures due to bugs not
currently fixed.
Comment 12 GStreamer system administrator 2018-11-03 15:27:58 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/457.