Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

Support for Pipe Input/Output #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nmorenor
Copy link

@nmorenor nmorenor commented Apr 1, 2023

Allow to display video from io.Reader/io.ReadSeeker in order to display video from other sources than file from disk.

@nmorenor nmorenor force-pushed the master branch 2 times, most recently from 278c5dd to 1aa67fa Compare April 1, 2023 20:24
@zergon321
Copy link
Owner

zergon321 commented Apr 2, 2023

I'm afraid I can't accept this PR in its current state for several reasons:

  • constants are named in the C style, not in the Go style (for example, IO_BUFFER_SIZE should be IOBufferSize);
  • why is the size of IO_BUFFER_SIZE 50000000? What if the user needs more or less than that? Why is it a constant? Moreover, I see that it doesn't need to be one. According to the docs, LibAV passes the size of the buffer provided by itself to the callback you specify. But you don't use that parameter in your C-exported functions. Instead you read IO_BUFFER_SIZE bytes from the input and copy all of them to the buffer provided by the LibAV caller even if it expects you to write much less than IO_BUFFER_SIZE. You should definitely copy only as much data as the callback caller wants (see the example);
  • in the current state of the PR, the user has to provide boilerplate handlers with a lot of CGO code. All the handlers code should be inside the library. The constructor should be NewMedia(src io.ReadWriteSeeker) (*Media, error). Yeah, if AVIO has to be integrated, the whole library should rely on it instead of just AV functions.

@nmorenor
Copy link
Author

nmorenor commented Apr 2, 2023

Hey, those are good points. And that opens up a bit the discussion.

  • The size of the buffer, it all started with different values, the issue was with the probe to infer the format of the data, it was throwing errors with other sizes, the error suggested to use at least that value that was when things started to work better.
  • Then I thought what if from the data stream you have, it can't determine the format, but user may want know/force a format. That is where I thought to allow end user to use an API to send the value back. This option sounds cool, but we could also simplify by sending a string of the format and look for it by name, but also having this option sounds cool. May the constructor could be the struct and if no handlers are specified internally have a default implementation? May be adding on the struct the option of the format as string? I'll circle down on this when I get a time.

@nmorenor
Copy link
Author

nmorenor commented Apr 6, 2023

I have pushed a small change trying to address these observations, it does feel a bit cleaner, the user of the library can optionally provide read, seek, writer handler methods, and adding a way to just send the readSeeker to get it to work, this opens up the library a bit on how it can be used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants