D13885: taglibextractor: Restore extracting audio props without tags existing

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:41c075129d46: taglibextractor: Restore extracting audio 
props without tags existing (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13885?vs=37205&id=37212

REVISION DETAIL
  https://phabricator.kde.org/D13885

AFFECTED FILES
  autotests/samplefiles/no-meta/test.flac
  autotests/samplefiles/no-meta/test.m4a
  autotests/samplefiles/no-meta/test.mp3
  autotests/samplefiles/no-meta/test.mpc
  autotests/samplefiles/no-meta/test.ogg
  autotests/samplefiles/no-meta/test.opus
  src/extractors/taglibextractor.cpp

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Alexander Stippich]

astippich edited the summary of this revision.
astippich edited the test plan for this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_empty_tags

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Matthieu Gallien]

mgallien accepted this revision.
mgallien added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R286 KFileMetaData

BRANCH
  fix_empty_tags

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Alexander Stippich]

astippich updated this revision to Diff 37205.
astippich added a comment.


  -fix the no-meta test case

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13885?vs=37148&id=37205

BRANCH
  fix_empty_tags

REVISION DETAIL
  https://phabricator.kde.org/D13885

AFFECTED FILES
  autotests/samplefiles/no-meta/test.flac
  autotests/samplefiles/no-meta/test.m4a
  autotests/samplefiles/no-meta/test.mp3
  autotests/samplefiles/no-meta/test.mpc
  autotests/samplefiles/no-meta/test.ogg
  autotests/samplefiles/no-meta/test.opus
  src/extractors/taglibextractor.cpp

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Alexander Stippich]

astippich commandeered this revision.
astippich edited reviewers, added: kossebau; removed: astippich.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: astippich, mgallien, michaelh, kossebau
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D13885#287042 , @astippich 
wrote:
  
  > 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 -f test.mp3` 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 :)

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Alexander Stippich]

astippich added a comment.


  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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D13885

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D13885: taglibextractor: Restore extracting audio props without tags existing

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: astippich, mgallien, michaelh.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added subscribers: Baloo, kde-frameworks-devel.
kossebau requested review of this revision.

REVISION 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 moves the existing files in a new subdir empty-meta/, to reflect they
  have some metadata but appear empty for the extraction. And adds a new test
  file with no metadata at all (for now only mp3 as I have no idea about
  metadata for the others formats)

TEST PLAN
  New no-meta/test.mp3 sample file no longer fails with the test.
  Unit test extractortest in baloo-widgets also works again, as audio metadata
  is extracted again from a file with no id3 data.

REPOSITORY
  R286 KFileMetaData

BRANCH
  restoreaudiopropextractionwithouttags

REVISION DETAIL
  https://phabricator.kde.org/D13885

AFFECTED FILES
  autotests/samplefiles/empty-meta/test.flac
  autotests/samplefiles/empty-meta/test.m4a
  autotests/samplefiles/empty-meta/test.mp3
  autotests/samplefiles/empty-meta/test.mpc
  autotests/samplefiles/empty-meta/test.ogg
  autotests/samplefiles/empty-meta/test.opus
  autotests/samplefiles/no-meta/test.flac
  autotests/samplefiles/no-meta/test.m4a
  autotests/samplefiles/no-meta/test.mp3
  autotests/samplefiles/no-meta/test.mpc
  autotests/samplefiles/no-meta/test.ogg
  autotests/samplefiles/no-meta/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

To: kossebau, astippich, mgallien, michaelh
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams