GNOME Bugzilla – Bug 734098
directsoundsink: gst_element_set_state blocked when plugout a usb audio device
Last modified: 2015-12-18 15:53:32 UTC
Overview: gst_element_set_state blocked when plugout a usb audio device. Steps to Reproduce: 1. Iinsert a usb device(like usb earphone). 2. Start to play music with gstdirectsoundsink. 3. Plugout the usb audio device. Actual Results: The application blocked when calling gst_element_set_state( .., NULL). Expected Results: The sound switch to play with the normal earphone. Platform: Built 2014-6-10 with mingw32. Source version 1.3.2 Additional Information I trace the code and found something more! static gint gst_directsound_sink_write (GstAudioSink * asink, gpointer data, guint length) { …… calculate_freesize: …… hRes = IDirectSoundBuffer_GetCurrentPosition (dsoundsink->pDSBSecondary, &dwCurrentPlayCursor, NULL); hRes = IDirectSoundBuffer_GetStatus (dsoundsink->pDSBSecondary, &dwStatus); if (SUCCEEDED (hRes) && (dwStatus & DSBSTATUS_PLAYING)) goto calculate_freesize; else { dsoundsink->first_buffer_after_reset = FALSE; GST_DSOUND_UNLOCK (dsoundsink); return 0; } …… } If usb earphone was plugged out, IDirectSoundBuffer_GetCurrentPosition return some error code, which will be overwrote by IDirectSoundBuffer_GetStatus. So the hRes seemed success every time. Treat the return values respectively, and the problem was soloved. Comment 1 Marcin Kolny (IRC: loganek) [gstreamermm-plugins-good developer] 2014-07-27 09:12:34 UTC Thanks for your report, but it seems to be GStreamer, not gstreamermm bug. Please report it in a correct project.
Why would GetStatus() still succeed? What is the return value that is returned by GetCurrentPosition() when the device is removed?
I'm sorry that I forgot what the return value was. And I don't have any usb audio device at hand now. No matter what the return value was, the code should not be written like that, each return value should be treated respectively.
(In reply to comment #1) > Why would GetStatus() still succeed? What is the return value that is returned > by GetCurrentPosition() when the device is removed? I'm sorry that I forgot what the return value was. And I don't have any usb audio device at hand now. No matter what the return value was, the code should not be written like that, each return value should be treated respectively.
True, but knowing that specific return value would allow to handle it specifically and do a generic error for all others. Can you check what the return value was? And do you want to provide a patch to fix the check in the code? :)
(In reply to comment #4) > True, but knowing that specific return value would allow to handle it > specifically and do a generic error for all others. Can you check what the > return value was? The func IDirectSoundBuffer_GetCurrentPosition return -2005401480(in %d format) when I plugged out the usb earphone. >And do you want to provide a patch to fix the check in the > code? :) I am pleasure to fix the bug, but when should I submit the patch to?:)
Just attach a patch, ideally in "git format-patch" format, to this bug report :)
That number seems to be DSERR_NODRIVER
*** Bug 736900 has been marked as a duplicate of this bug. ***
Created attachment 288121 [details] [review] A patch for the bug My first time to create a patch. I don't know if this patch is OK
Sorry to reply so late.
(In reply to comment #10) > Sorry to reply so late. Thanks for the Patch. I think check may have to be done at more places at lines 574, 677 - 680, 692, 745, .. Please refer to below bugID https://bugzilla.gnome.org/show_bug.cgi?id=736900
Created attachment 288210 [details] [review] A improved patch for the bug Add check for some more place. I think that the first two hRes has no need to be checked, since it's just a reset.
A bug of directsoundsrc is almost the same as this one. See https://bugzilla.gnome.org/show_bug.cgi?id=738292
Review of attachment 288210 [details] [review]: If you update the patch, please attach it in "git format-patch" format with a proper commit message, and the name/mail details set up in GIT :) In general looks good, just some minor comments. Thanks for the patch! ::: gstdirectsoundsink.c @@ +541,3 @@ GstDirectSoundSink *dsoundsink; DWORD dwStatus; + HRESULT hRes, hRes2; Can you implement this with a single variable instead? @@ +576,3 @@ &dwCurrentPlayCursor, NULL); + hRes2 = e.g. here it should probably go out of the loop after the first error already
Yongjian Xu, are you planning on updating the patch ?
Created attachment 317195 [details] [review] directsoundsink: fix Bug 734098 gst_element_set_state blocked when plugout a usb audio device and also fix Bug 678415
commit 4d72fd98842f519c2ec3c0352642b159ffc6ca57 Author: Thomas Roos <thomas.roos@industronic.de> Date: Fri Dec 11 11:23:13 2015 +0100 directsoundsink: Check the return value of GetStatus() too to decide if there was an error If GetStatus() fails, the status itself won't be very meaningful but we also have to look at its return value. This fixes blocking pipelines when removing sound devices or during other errors, where we wouldn't notice the error and then wait forever. https://bugzilla.gnome.org/show_bug.cgi?id=734098
*** Bug 678415 has been marked as a duplicate of this bug. ***