Handle scan results without duration information more gracefully

[tomka]

While scanning my music library, I had mopidy-local crash on some files. The reason was that the respective scan results came without duration information. That's unfortunate, but at least happens rarely it seems.

It is currently assumed that a playable scan result has always duration information. As said above, that's not always the case. I believe it would be valuable to handle this this corner case more gracefully, it would make mopidy-local more robust. Therefore, this patch makes the meta-data scan now ignore scan results without duration information. I tested this on my local music library and was now able to scan it without further problems.

Alternatively to suggested change, Scanner.scan() could raise a ScannerError if the duration field is None. The Scanner implementation suggests to me though that it is explicitly wanted that duration can be None to represent an error case.

Also, the except statement could additionally catch all other error types and continue with the next field, to not cancel the whole scan in case of errors with individual files. I know to little about the overall design if this is a reasonable suggestion though.

[codecov[bot]]

Codecov Report

Merging #35 into master will decrease coverage by 0.13%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   50.52%   50.39%   -0.14%     
==========================================
  Files           9        9              
  Lines         762      764       +2     
==========================================
  Hits          385      385              
- Misses        377      379       +2

Impacted Files	Coverage Δ
mopidy_local/commands.py	0% <0%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0720824...29c9553. Read the comment docs.

[kingosticks]

Thanks for taking time to provide the thorough explanation and reasoning in this PR, very helpful.

Just as some background, I think we get a None duration from some file/codec types even though they do actually play (thanks to some GST bug that may or may not still exist in 1.14), and that's why the Scanner allows this situation rather than erroring. Particularly in the case of Internet radio streams, which also use the Scanner in Mopidy-Stream. @adamcik might remember better.

But that doesn't stop this change being an improvement, looks good to me also.

[tomka]

I force-pushed an update that passes the tests now. Thanks for the additional explanation on when to expect None as duration and why scanner allows this. I checked the files that caused this for me and they are all "Musepack" (.mpc) files.

[kingosticks]

I checked the files that caused this for me and they are all "Musepack" (.mpc) files.

Out of interest, does GStreamer play them if you browse to them using Mopidy-File? Gstreamer does seem to support Musepack. EDIT: Try the gst-discoverer-1.0 suggestion below instead, that is much easier.

[tomka]

Out of interest, does GStreamer play them […]?

Yes, I can play the .mpc file with mopidy-file. My MPD client (ncmpcpp) shows however no song length for the .mpc files (it shows (-:--) instead).

If I run gst-discoverer-1.0 $uri_or_path_to_file with an .mpc file that I can play through mopidy-file, it crashes with a segmentation fault. These are the top ten levels of the call stack in the generated core dump:

#0  0x00007f25374e013d in gst_caps_get_structure () at /usr/lib/libgstreamer-1.0.so.0   
#1  0x00007f253775cd52 in  () at /usr/lib/libgstpbutils-1.0.so.0                        
#2  0x00007f253775d077 in  () at /usr/lib/libgstpbutils-1.0.so.0                        
#3  0x00007f253709b69a in ffi_call_unix64 () at /usr/lib/libffi.so.6                    
#4  0x00007f253709afb6 in ffi_call () at /usr/lib/libffi.so.6
#5  0x00007f25375955ae in g_cclosure_marshal_generic () at /usr/lib/libgobject-2.0.so.0 
#6  0x00007f253758bd5a in g_closure_invoke () at /usr/lib/libgobject-2.0.so.0           
#7  0x00007f253757988e in  () at /usr/lib/libgobject-2.0.so.0
#8  0x00007f253757d98a in g_signal_emit_valist () at /usr/lib/libgobject-2.0.so.0       
#9  0x00007f253757e7f0 in g_signal_emit () at /usr/lib/libgobject-2.0.so.0              
#10 0x00007f25374dad56 in gst_element_add_pad () at /usr/lib/libgstreamer-1.0.so.0      

I guess I should file a bug with the gstreamer project.

[tomka]

I guess I should file a bug with the gstreamer project.

And so I did, for reference: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/498