After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734098 - directsoundsink: gst_element_set_state blocked when plugout a usb audio device
directsoundsink: gst_element_set_state blocked when plugout a usb audio device
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Windows
: Normal normal
: 1.6.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 678415 736900 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-08-01 08:57 UTC by Yongjian Xu
Modified: 2015-12-18 15:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
A patch for the bug (2.22 KB, patch)
2014-10-09 12:29 UTC, Yongjian Xu
none Details | Review
A improved patch for the bug (3.13 KB, patch)
2014-10-10 11:29 UTC, Yongjian Xu
needs-work Details | Review
directsoundsink: fix Bug 734098 gst_element_set_state blocked when plugout a usb audio device (2.99 KB, patch)
2015-12-11 10:29 UTC, Thomas Roos
committed Details | Review

Description Yongjian Xu 2014-08-01 08:57:09 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.
Comment 1 Sebastian Dröge (slomo) 2014-08-01 12:02:07 UTC
Why would GetStatus() still succeed? What is the return value that is returned by GetCurrentPosition() when the device is removed?
Comment 2 Yongjian Xu 2014-08-20 00:27:18 UTC
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.
Comment 3 Yongjian Xu 2014-08-20 00:27:47 UTC
(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.
Comment 4 Sebastian Dröge (slomo) 2014-08-25 07:42:16 UTC
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? :)
Comment 5 Yongjian Xu 2014-09-04 11:43:15 UTC
(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?:)
Comment 6 Sebastian Dröge (slomo) 2014-09-04 12:14:02 UTC
Just attach a patch, ideally in "git format-patch" format, to this bug report :)
Comment 7 Sebastian Dröge (slomo) 2014-09-04 12:15:58 UTC
That number seems to be DSERR_NODRIVER
Comment 8 Sanjay NM 2014-10-09 05:30:24 UTC
*** Bug 736900 has been marked as a duplicate of this bug. ***
Comment 9 Yongjian Xu 2014-10-09 12:29:08 UTC
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
Comment 10 Yongjian Xu 2014-10-09 12:29:34 UTC
Sorry to reply so late.
Comment 11 Sanjay NM 2014-10-10 04:11:44 UTC
(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
Comment 12 Yongjian Xu 2014-10-10 11:29:25 UTC
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.
Comment 13 Yongjian Xu 2014-10-10 11:53:08 UTC
A bug of directsoundsrc is almost the same as this one.
See https://bugzilla.gnome.org/show_bug.cgi?id=738292
Comment 14 Sebastian Dröge (slomo) 2014-10-14 08:19:26 UTC
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
Comment 15 Tim-Philipp Müller 2014-12-13 13:47:03 UTC
Yongjian Xu, are you planning on updating the patch ?
Comment 16 Thomas Roos 2015-12-11 10:29:46 UTC
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
Comment 17 Sebastian Dröge (slomo) 2015-12-11 10:43:02 UTC
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
Comment 18 Sebastian Dröge (slomo) 2015-12-11 10:43:31 UTC
*** Bug 678415 has been marked as a duplicate of this bug. ***