Winforms: DataObject.SetAudio should be exposed with a new overload taking Span<byte>

[raffaeler]

The DataObject class currently exposes two audio overloads. The first one taking a byte[] calls the second who takes a Stream.

Rationale

Since the introduction of Span<T> it become preferable exposing APIs using Span<T> because they are stack-only types minimizing the use of the garbage collector and easily allows slicing the referenced memory.

Proposed API

```C#
namespace System.Windows.Forms
{
public partial class DataObject : IDataObject
{
// Existing methods
public virtual void SetAudio(byte[] audioBytes);
public virtual void SetAudio(Stream audioStream);

    // Proposed method
    public virtual void SetAudio(Span<byte> audioBytes);
}

}

An additional advantage of `Span<T>` is that it self describes the contiguous memory to be processed from the API. The existing overload taking `byte[]` does not even allow to specify an offset and length of the array.

Since the current overloads allow specifying an empty interval and only check whether the parameter (`byte[]` or `Stream` is null), it is also possible to avoid any parameter check since `Span<T>` is a `ref struct` and cannot be null.
The parameter `Span<byte>` can be empty, as it is for the other overloads.

## Implementation
The proposed implementation of the new public API is the following:
```c#
public virtual void SetAudio(Span<byte> audioBytes)
{
    var stream = new MemoryStream(audioBytes.Length);
    stream.Write(audioBytes);
    stream.Seek(0, SeekOrigin.Begin);
    SetAudio(stream);
}

The MemoryStream is not disposed as it is not directly consumed from the called methods. The other overload does the same.

The PR for this new API is available and contains also a test with four different types of blobs, including an empty one.

[karelz]

Thanks @raffaeler - this is helpful for context!
cc @zsd4yr @JeremyKuhne

[zsd4yr]

This is great! Still ironing out what it will mean to expose an additional overload api using span when span is not supported in VB. This is also an ongoing discussion in #148; hopefully we'll get an answer from our api review board and amongst the team sometime this next week.

[zsd4yr]

@raffaeler once you feel comfortable with your work here (including testing 😄 ), feel free to mark this issue as api-ready-for-review. I'm working on next-steps with our API Review board

[raffaeler]

@zsd4yr As we already discussed the topic with @karelz and I did further investigations, I believe it's done, unless you have comments in the linked PR.
Said that, I have no clue on how to assign that label :) I believe you only can do that.

[zsd4yr]

Assiging us both for my own purposes 😀

[danmosemsft]

I don't have context on this API/scenario, but note that on the BCL we are trying to be quite selective with Span overloads - at an extreme we could add thousands, such as for every string parameter, which would bloat code, docs, and intellisense. We are adding them only when it delivers signifcant perf advantages, and the developer using the API is likely to be comfortable with using Spans. (That's not the official statement, it's my words). In this case if it's typical to have to copy memory into a byte[] then adding Span support would be an efficiency gain, but if they were normally read from disk via a Stream then it perhaps would not.

[raffaeler]

@danmosemsft I am not sure to understand the rationale in what you are saying. Most of nestandard2.1 is about overloads taking Span<> or ReadOnlySpan<>. Many of them are for interop with Streams, I/O related classes, StringBuilder and many other (I can count 706 at the provided link).

I am not saying that we should bloat the entire winforms codebase with Span<>, but just to add a number of overloads that give the developers not only to use that, but also to get more familiar with a pattern that is definitely helpful and neat.
This proposal is just about an overload that have the advantage of removing an allocation from the user codebase and to "teach" the developer that is a good idea to use Span<> instead of a byte[].

[MichalStrehovsky]

The litmus test for adding a Span overload is to answer the question: can this be used to avoid an allocation? The implementation provided in #217 does a full copy of the span's content into a stream. The user might call this API thinking they're avoiding an allocation, but the API will allocate nonetheless (the provided implementation makes a copy of the span's data and stores it in a MemoryStream).

It really is just a shortcut for SetAudio(mySpan.ToArray()) (calling ToArray would have been more efficient in #217 implementation too).

The only question is whether some other class deriving from DataObject could override the new overload and make it actually allocation free.

[raffaeler]

@MichalStrehovsky Unfortunately I could not get rid of the Stream, but I believe there is another point here.
The original implementation does not allow to specify the offset and length parameter. Therefore, if the user is willing to slice the buffer, the Span overload can avoid the additional allocation.
Now the problem is if we should add a public virtual void SetAudio(byte[] audioBytes, int offset, int count) overload to make VB developers happy.

[MichalStrehovsky]

The original implementation does not allow to specify the offset and length parameter. Therefore, if the user is willing to slice the buffer, the Span overload can avoid the additional allocation.

```c#
var slice = mySpan.Slice(...);
do.SetAudio(slice);

The proposed implementation of the new `SetData` overload will allocate an array that has the same length as the slice within the `MemoryStream` for the above snippet.

This is equivalent to the allocation incurred for the following piece of code that is already possible with the existing API surface:

```c#
var slice = mySpan.Slice(...);
do.SetAudio(slice.ToArray());

Am I missing something?

[raffaeler]

I was said that VB developers cannot take advantage of the Span<> because of language limitations. So I was wondering if the other byte[], int, int should be evaluated or not.

[MichalStrehovsky]

I was said that VB developers cannot take advantage of the Span<> because of language limitations. So I was wondering if the other byte[], int, int should be evaluated or not.

I'll let owners of WinForms speak to that, but this looks like a convenience method too.

I assume VB developers would have the data in an array.

They can do:

var ms = new MemoryStream(arr, index: 2, count: 2);
do.SetData(ms);

This would even prevent the copy of the data for the slice because MemoryStream uses the provided array as the backing store (no copy is made).

[terrajobst]

I'll let owners of WinForms speak to that, but this looks like a convenience method too.

Agreed, and that follows largely how the BCL is designed to.

[terrajobst]

Video

We aren't experts on IDataObject but what is the benefit of using Span<T> here? The Stream API allows you to slice the data already, and it seems the implementation pushes the state into the heap.

For this scenario Span<T> doesn't make sense; at the very minimum it would need to be Memory<T> (because Span<T> cannot be stored on the heap). However, this seems to complicate the API surface more than it helps.

If slicing the array would be common, I suggest we instead add an overload that simply accepts two ints, or an ArraySegment<T>.