GNOME Bugzilla – Bug 794902
qtdemux tests: Add edit list test battery
Last modified: 2018-11-03 15:27:58 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.
Created attachment 370443 [details] [review] qtdemux tests: Add edit list test battery
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
Thanks! I'm not sure the dependency on node will be generally liked, but having tests for this is certainly a good idea.
I've made a GitHub branch with all these patches together: https://github.com/ntrrgc/gst-plugins-good/tree/20180402-edit-lists-tests-squash
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.
(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?
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.
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.
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.
(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.
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.
-- 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.