Skip to content

Commit

Permalink
clone font and dont use string literals (C# 11 feature)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lay295
lay295 committed Dec 11, 2022
1 parent 2def0e9 commit 86c9503
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 3 deletions.
5 changes: 3 additions & 2 deletions TwitchDownloaderCore/ChatRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ private void DrawMessage(Comment comment, List<SKBitmap> sectionImages, List<(Po
DrawText(messageBuffer.ToString(), messageFont, true, sectionImages, ref drawPos, defaultPos);
messageBuffer.Clear();
}
SKPaint fallbackFont = GetFallbackFont(char.ConvertToUtf32(fragmentChars[j], fragmentChars[j + 1]), renderOptions);
using SKPaint fallbackFont = GetFallbackFont(char.ConvertToUtf32(fragmentChars[j], fragmentChars[j + 1]), renderOptions).Clone();
fallbackFont.Color = renderOptions.MessageColor;
DrawText(fragmentChars[j].ToString() + fragmentChars[j + 1].ToString(), fallbackFont, false, sectionImages, ref drawPos, defaultPos);
j++;
Expand All @@ -603,7 +603,7 @@ private void DrawMessage(Comment comment, List<SKBitmap> sectionImages, List<(Po
DrawText(messageBuffer.ToString(), messageFont, true, sectionImages, ref drawPos, defaultPos);
messageBuffer.Clear();
}
SKPaint fallbackFont = GetFallbackFont(fragmentChars[j], renderOptions);
using SKPaint fallbackFont = GetFallbackFont(fragmentChars[j], renderOptions).Clone();
fallbackFont.Color = renderOptions.MessageColor;
DrawText(fragmentChars[j].ToString(), fallbackFont, true, sectionImages, ref drawPos, defaultPos);
}
Expand Down Expand Up @@ -824,6 +824,7 @@ private void DrawUsername(Comment comment, List<SKBitmap> sectionImages, ref Poi
userPaint.Color = userColor;
sectionImageCanvas.DrawText(comment.commenter.display_name + ":", drawPos.X, drawPos.Y, userPaint);
drawPos.X += textWidth + renderOptions.WordSpacing;
userPaint.Dispose();

This comment has been minimized.

Sign in to view

Copy link
@ScrubN

ScrubN Dec 11, 2022

Collaborator

You just reintroduced #448. We can't dispose because userPaint is a shallow copy.

This comment has been minimized.

Sign in to view

Copy link
@lay295

lay295 Dec 11, 2022

Author Owner

I was getting the error as well, so I just clone the fallback font we get. We'll be disposing the copy which should be fine (it fixed it for me)

This comment has been minimized.

Sign in to view

Copy link
@lay295

lay295 Dec 11, 2022 •
edited
Loading

Author Owner

The clone as added in an earlier commit (I didn't notice you fixed #448 in the PR). We have to modify the SKPaint we get anyways so I think the current approach of just cloning it, modifying the clone, and disposing it makes sense. Where before we would just take a reference to the font and keep modifying that single object (and when we disposed it, caused issues later)

(Also if you're reading this 1.50.8 is just marked as a prerelease for now, because I'm updating my cloudflare worker to point to the correct file. The renaming messed it up a little.)

This comment has been minimized.

Sign in to view

Copy link
@ScrubN

ScrubN Dec 11, 2022 •
edited
Loading

Collaborator

I'm pretty sure .Clone() just returns a pointer to the SKPaint object data.

// Summary:
//     Creates a copy of the current paint.
//
// Returns:
//     Returns the copy.
//
// Remarks:
//     The copy is a shallow copy, all references will still point to the same objects.
public SKPaint Clone()

The only chatrender change I made in the commit that fixed this issue was removing that exact dispose line:
ScrubN@bc9d090#diff-ca35d49087eef863cf91c18f7d2fe213631ffee6a5994e72d1a703348b37d734

The reason it didn't error again during your testing is likely because of frame reuse, but it is entirely possible for other scenarios to occur that cause the seg fault again

This comment has been minimized.

Sign in to view

Copy link
@lay295

lay295 Dec 12, 2022

Author Owner

Hmmmm, I'm unsure. I undid my previous frame re-use change and I remove cloning of the fallback font and I do get the crash on this json for example.

Adding the clone back, I do not get a crash. (with the dispose still there)
userPaint = GetFallbackFont(comment.commenter.display_name.Where(IsNotAscii).First(), renderOptions).Clone();

10minjap.txt

This comment has been minimized.

Sign in to view

Copy link
@ScrubN

ScrubN Dec 12, 2022

Collaborator

I see. I don't know why we were crashing then, we were always cloning.

This comment has been minimized.

Sign in to view

Copy link
@lay295

lay295 Dec 12, 2022

Author Owner

It says it's a shallow clone in the documentation, but looking at the Handle of when I clone it seems to be a different handle.

Handle | Gets or sets the handle to the underlying native object.(Inherited from SKObject)

I see when I clone the SKPaint I get a different Handle.

(Before clone)
image

(After clone)
image

This comment has been minimized.

Sign in to view

Copy link
@lay295

lay295 Dec 12, 2022 •
edited
Loading

Author Owner

I see. I don't know why we were crashing then, we were always cloning.

We weren't, In this commit I added the clone ec4d8d4

It was my own version of the fix (because I didn't see you also made a fix in your PR). I wasn't aware of issue #448 but I ran into the issue on my own testing my frame re-use commit with the Test.json from #456

This comment has been minimized.

Sign in to view

Copy link
@ScrubN

ScrubN Dec 12, 2022

Collaborator

I see, very good to know.

I'm going to start taking the SkiaSharp docs with a grain of salt due to the amount of unfinished cases or inaccurate information.
}

private static SKColor GenerateUserColor(SKColor userColor, SKColor background_color, ChatRenderOptions renderOptions)
Expand Down
2 changes: 1 addition & 1 deletion TwitchDownloaderCore/TwitchHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ public static async Task<string> GetStreamerName(int id)
{
RequestUri = new Uri("https://gql.twitch.tv/gql"),
Method = HttpMethod.Post,
Content = new StringContent($$$"""{"query":"query{user(id:\"{{{id}}}\"){login}}","variables":{}}""", Encoding.UTF8, "application/json")
Content = new StringContent("{\"query\":\"query{user(id:\\\"" + id.ToString() + "\\\"){login}}\",\"variables\":{}}", Encoding.UTF8, "application/json")
};
request.Headers.Add("Client-ID", "kimne78kx3ncx6brgo4mv6wki5h1ko");
string response = await (await httpClient.SendAsync(request)).Content.ReadAsStringAsync();
Expand Down

0 comments on commit 86c9503

Please sign in to comment.