Fix thumbnails issues

[avelad]

The commit 264c842 introduce 2 issues:

• stream.segmentIndex is not always constructed (eg: DASH)
• In Image track the size of the thumbnail itself is returned and not the size of the complete image, so it is necessary to return the size of the complete image in getThumbnails so that later the UI can correctly place the image if the layout is different to 1x1.

[avelad]

@theodab can you review?

[joeyparrish]

Why do we need imageHeight and imageWidth? Isn't height/width and offset x, y enough?

[avelad]

@joeyparrish , we need the imageHeight and imageWidth because the thumbnail height/width and the offset don't allow us calculate the full image size. Also before the commit 264c842 the getImageTracks returned the full image size.

[joeyparrish]

I see that it doesn't allow you to compute the full image size, but if you only need to display a single thumbnail, I don't understand why you need the full image size in the first place. I'm not against providing it, but I want to understand the purpose.

[avelad]

@joeyparrish
If the layout is 1x1, I don't need this information, because the size of the full image and the thumbnail is the same.
If the layout is different of1x1, I want to paint a thumbnail of AxB in a 640x360 rectangle, but the image size is CxD (different from 640x260), so I need to know the scale to be able to adjust the measurements, and crop the full image to show only the specific thumbnail. (see #3371 (comment))

[avelad]

I am seeing that this PR has not fired github action, why? 😮

[joeyparrish]

I'm not sure why. We have only very recently deployed a GitHub Actions workflow, so I'm sure we will still be surprised and have more to learn. I'll look into it now.

[joeyparrish]

I can't find any reason for the PR workflow to be skipped. I also can't find any way to trigger it in a way that is linked to this PR. I was able to trigger it manually to run against your commit, but it isn't linked back to this PR. I'm going to try closing and re-opening the PR to see if that triggers the workflow.

[joeyparrish]

That seems to have worked. I'm really not sure why it didn't work correctly the first time. I'll double-check our workflow triggers for mistakes.

[avelad]

Maybe a usage limit? https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

[theodab]

I was going to suggest an alternative approach, which involved making Player.getImageTracks async so that we can create the segment index if necessary.

However, you are right that the segment references are only assigned tilesLayout values by the HLS parser, at the moment. So changing the interface so that we guaranteed can check the segment references would be future-proofing, at best. Probably not worth the hassle of changing our public interface over.

Given that, I think that your approach here is probably ideal.

[avelad]

@theodab I do not agree to do Player.getImageTracks async, that would be a breaking change and it should go in 4.0, since we are doing a fix of an existing functionality, we should not change Player.getImageTracks so that it returns a promise.

[theodab]

Oh, I guess it needs to be:

tilesLayout: layout || null,

Since the stream's tilesLayout is string|undefined, not ?string.

[avelad]

Fixed, I used my iPad for the commit instead of turning on the mac ... sorry

[joeyparrish]

@theodab, sorry for re-requesting your review here. The page was not refreshed, and you had already approved and merged it. Please disregard.