Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature request: Add side_data attribute to Stream #1150

Closed
hyenal opened this issue Aug 8, 2023 · 10 comments · Fixed by #1249
Closed

Feature request: Add side_data attribute to Stream #1150

hyenal opened this issue Aug 8, 2023 · 10 comments · Fixed by #1249

Comments

@hyenal
Copy link
Contributor

hyenal commented Aug 8, 2023

Overview

Sometimes a stream encodes side data in the entire stream (in side_data). Some of those side data are quite essential to read the video correctly.
For instance displaymatrix sometimes encode whether the video is rotated or not.

From the AV documentation it seems like we could know easily if there is side data with the attribute nb_side_data.

I would like to know if this is an interesting feature for this library ? I started a fork there with, for now, a simple skeleton of how it would look like

Desired Behavior

When reading a video (or any other stream) you would get the side data as a dictionary:

# This would give you the stream metadata
container.stream.video[0].side_data
@hyenal
Copy link
Contributor Author

hyenal commented Aug 9, 2023

I made this PR on my own fork. It only fetch the DISPLAYMATRIX but would be happy to implement more and submit a PR to this repo if anyone is interested

@lgeiger
Copy link
Contributor

lgeiger commented Nov 21, 2023

@hyenal I'd be really interested in having this functionality upstreamed to av as it would fix #1045 is a blocker for me currently and also many people seem to run into.

@jlaine or @WyattBlue are probably in a better position to judge how this should be implemented best to make it consistent with the way frame side data is handled. I'm happy to help out and write some test cases if that would be helpful.

@hyenal
Copy link
Contributor Author

hyenal commented Nov 21, 2023

Thanks @lgeiger, so happy to see this getting momentum!

It looks like it's been merged in @WyattBlue repository and will be available in PyAv soon?

I am also happy to write some tests if this helps :)

@lgeiger
Copy link
Contributor

lgeiger commented Nov 21, 2023

It looks like it's been merged in @WyattBlue repository and will be available in PyAv soon?

Yes, it's already available on pypi though this causes dependency conflicts for me as I am relying on a different package that requires av directly which is why it would be great to get upstreamed at some point.

@hyenal
Copy link
Contributor Author

hyenal commented Nov 22, 2023

Yes, it's already available on pypi though this causes dependency conflicts for me as I am relying on a different package that requires av directly which is why it would be great to get upstreamed at some point.

I see thanks for the explanation!

Let's wait for an answer from maintainers then, I am hopeful this could be merged 🙏

@WyattBlue
Copy link
Member

Sure, @hyenal Make a new PR for this repo.

@hyenal
Copy link
Contributor Author

hyenal commented Nov 27, 2023

I'm happy to help out and write some test cases if that would be helpful.

@lgeiger do you have some tests in mind ? As far as I understood we rely on FATE to get video samples, I hope those have a sample with a non zero display matrix so that we could write a test easily.
Otherwise the remaining work I had in mind was to make sure stream.sidedata works and polish the code (removing the two TODOs)

I intend to do this next week so if you it's a blocker for you I d be happy to invite you as a contributor to my PR :)

@lgeiger
Copy link
Contributor

lgeiger commented Nov 27, 2023

@lgeiger do you have some tests in mind ? As far as I understood we rely on FATE to get video samples, I hope those have a sample with a non zero display matrix so that we could write a test easily.

Not really. I probably could share some example videos but I guess it would be better to get them from FATE.

I intend to do this next week so if you it's a blocker for you I d be happy to invite you as a contributor to my PR :)

That's great, thanks for working on it. It's not a blocker for me anymore. For reference, we ended up using pymediainfo to read the rotation information from the video files and handle them accordingly via Numpy which works well for our use case but it would still be great if this would be natively supported in av.

@WyattBlue WyattBlue linked a pull request Dec 20, 2023 that will close this issue
@WyattBlue
Copy link
Member

We're planning to remove Stream.side_data (#1570) in PyAV 14 since FFmpeg doesn't even document AVStream.side_data or AVStream.nb_side_data anymore.

I think there will be some alternative API, but we're waiting for FFmpeg 8 to be released.

@WyattBlue WyattBlue closed this as not planned Won't fix, can't repro, duplicate, stale Oct 10, 2024
@lgeiger
Copy link
Contributor

lgeiger commented Nov 12, 2024

@WyattBlue thanks for the heads up. What's the recommended way of checking whether the video content needs to be rotated with PyAV?

As far as I can tell without side_data we won't know whether the frame needs to be rotated (e.g. when we need to do further processing of the numpy frame).

Of course this can be checked via ffprobe -loglevel error -select_streams v:0 -show_entries stream_tags=rotate -of default=nw=1:nk=1 video.mp4 or using something like pymediainfo but that means adding another requirement which tends to complicate things.

As far as I can tell correct handling of rotation is the reason why some upstream packages still rely on av < 10 (see #1045).

@PyAV-Org PyAV-Org locked and limited conversation to collaborators Nov 13, 2024
@WyattBlue WyattBlue converted this issue into discussion #1629 Nov 13, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants