taglibextractor: Restore extracting audio props without tags existing
ClosedPublic

Authored by astippich on Jul 4 2018, 3:37 PM.

Details

Summary

7f9de32eedff7f81818145001fc38bbed01de1b7 made extraction of audio
properties depending on the presence of any tag, by moving the extraction
into the "if (!tags->isEmpty())" branch.
This dependency is not necessary and prevents extracting the audio data
for files without a tag (as sideeffect broke a test in baloo-widgets).

Existing sample autotest files in no-meta/ subdir have not catched this,
as they still had an ID3 tag content (TSSE), so tags->isEmpty() was false.

Patch removes all metadata from the existing no-meta files, and fixes the resulting test failures

Test Plan

Tests wit the updated no-meta test files do no longer fail
Unit test extractortest in baloo-widgets also works again, as audio metadata
is extracted again from a file with no id3 data.

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Jul 4 2018, 3:37 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptJul 4 2018, 3:37 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Jul 4 2018, 3:37 PM

Sorry for causing the regressions and thanks for the fix!
I just checked all the no-meta files. The reason that they did not cause the tests to fail is that they still have at least one tag defined that is not read (encoder settings for example). I think it would be better to completely remove the tags from the no-meta files instead of adding another test file.
You can easily do that with the kid3 tag editor, but I can also do that if you prefer.

kossebau added a comment.EditedJul 4 2018, 8:37 PM

I just checked all the no-meta files. The reason that they did not cause the tests to fail is that they still have at least one tag defined that is not read (encoder settings for example).

Okay, so can confirm that what I wrote in the description/summary is correct ;)

I think it would be better to completely remove the tags from the no-meta files instead of adding another test file.

Fine with me. I did not spent time thinking about whether the almost tag-empty files are covering proper test cases or if they should have been really empty, as in tag-free :)

You can easily do that with the kid3 tag editor, but I can also do that if you prefer.

I used id3v2 -D test.mp3 (fixed: -D, not -f) to create the test.mp3 without any id tags from the existing :) But had to goggle up how to do that, so happy to leave this to people who have experience :)
So happy to have you take over this patch, all I want is to have the tests fixed :)

astippich commandeered this revision.Jul 5 2018, 7:37 PM
astippich edited reviewers, added: kossebau; removed: astippich.
astippich updated this revision to Diff 37205.Jul 5 2018, 7:38 PM

-fix the no-meta test case

All updated no-meta samplefiles fail for me without the patch, but as wanted do not fail with the patch. So seems fine to me :) @astippich Thanks for picking up this patch and creating proper test files.
+1 though only, given I am no maintainer.

mgallien accepted this revision.Jul 5 2018, 8:30 PM

@kossebau thanks for your work on this patch
@astippich thanks for your quick reaction and the work on this patch

This revision is now accepted and ready to land.Jul 5 2018, 8:30 PM
astippich edited the summary of this revision. (Show Details)Jul 5 2018, 9:15 PM
astippich edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.