Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

added video file validation and optimization when publishing streams (using --validate_file and --optimize_file arguments) #2726

Merged
merged 9 commits into from
Feb 4, 2020

Conversation

BrannonKing
Copy link
Member

@BrannonKing BrannonKing commented Jan 14, 2020

Please review and ask questions. Use the script to test files.

Fixes #1988

@kauffj
Copy link
Member

kauffj commented Jan 15, 2020

If the plan is for lbry-desktop to be the first place this is used by end-users, the app team will need a way to know if this feature is available on the system. Jamming it into status would be fine if it's not costly to report.

@BrannonKing BrannonKing force-pushed the add_video_transcoding branch 4 times, most recently from a3e41f2 to 36851a5 Compare January 16, 2020 23:20
@BrannonKing
Copy link
Member Author

BrannonKing commented Jan 27, 2020

Instructions for using this:

There are two new parameters to the RPC methods stream_create and publish.
--validate_file will force the incoming video file to meet various parameters.
--optimize_file will attempt to fix failing file validation.
The parameters are not mutually exclusive. If you both validate and optimize we will error out if the optimization attempt fails to bring things within spec (which should be very rare).

Things we validate:

  1. container is of type mp4, webm, or ogg.
  2. video codec is h264 or vpx or av1 or theora.
  3. audio codec is aac or opus or vorbis or mp3 or flac.
  4. audio codec corresponds to video codec
  5. bitrate isn't too horrible
  6. audio volume isn't too soft
  7. video codec pixel format is yuv420p when codec is h264
  8. faststart flag is set for h264

The audio volume check and repair is the most problematic in that it's hard to correct in a single pass. It would be much better for the user to do that on their own time. You can disable the audio volume check by setting the volume_analysis_time to 0 in the conf file.

You can use a script to test things without running the full SDK. It's called video_checker.py and lives in the SDK's scripts folder.

At present, the SDK creates a "fixed" video file next to the one that the user selected. It does not remove the fixed video. The user can use that for comparison to the original. We don't currently have events in place to allow us to delete the file once it has been successfully published.

There are some options that can be manually configured in the conf file. Here's a cut of the code for that:

    ffmpeg_folder = String('The path to ffmpeg and ffprobe', '')
    video_encoder = String('FFmpeg codec and parameters for the video encoding. '
                           'Example: libaom-av1 -crf 25 -b:v 0 -strict experimental',
                           'libx264 -crf 18 -vf "format=yuv420p"')
    audio_encoder = String('FFmpeg codec and parameters for the audio encoding. '
                           'Example: libopus -b:a 128k',
                           'aac -b:a 192k')
    volume_filter = String('FFmpeg filter for audio normalization.', '-af loudnorm')
    volume_analysis_time = Integer('Maximum seconds into the file that we examine audio volume (0 to disable).', '240')

@kauffj
Copy link
Member

kauffj commented Jan 27, 2020

Is the expectation that this PR will include everything sufficient to add a checkbox to lbry-desktop on publish?

@BrannonKing
Copy link
Member Author

@kauffj , yes this will be sufficient for checkbox in the app. I think the question of what to do with the transcoded file is still lingering, but that may linger until there is an event system in the SDK that can let us know when a file is no longer needed.

@BrannonKing
Copy link
Member Author

I also want some additional testing on the audio volume handling, to know if we want to ship with that feature on or off.

@BrannonKing
Copy link
Member Author

@lyoshenka proposes that we name the parameters "check_browser_compatibility" and "optimize_for_browser_compatibility". Any others for or against?

@eukreign
Copy link
Member

@kauffj depends, does the app need to know if transcoding capability is available before showing the checkbox option?

@kauffj
Copy link
Member

kauffj commented Jan 29, 2020

Yes: #2726 (comment)

@BrannonKing BrannonKing force-pushed the add_video_transcoding branch 2 times, most recently from a791b75 to 50ec32c Compare January 29, 2020 01:31
@tzarebczan
Copy link
Contributor

Did some testing and debugging with Brannon on Windows yesterday and it can't find ffmpeg/ffprobe, even when setting the config path directly. Brannon will take another look.

@BrannonKing BrannonKing force-pushed the add_video_transcoding branch 3 times, most recently from 977ccf3 to d03204b Compare January 30, 2020 17:37
@BrannonKing
Copy link
Member Author

what would be the advantage of adding a separate field to status over simply checking for a minimum SDK version number?

@tzarebczan
Copy link
Contributor

You'd still want to know if someone has the ffmpeg binaries in place before having them check the box.

@kauffj how do you feel about having something in the UI that will guide them to a download, and extract the binaries to the sdk folder (which would be set in the config). Do we want the first version without that? I think it would be easier if we at least told people "put it in this directory" vs having them add it to path or having them configure the ffmpeg path in settings.

@kauffj
Copy link
Member

kauffj commented Jan 30, 2020

I do not think the SDK version number guarantees ability to transcode (will lbry.tv be able to do it? will Android?).

I'm all for good instructions and lots of other things to make this more accessible, I'm just looking, as always, to ship something as soon as possible. It seems like simply only showing the box if it's already on the system and callable is faster than figuring out how to bundle, auto-install, handhold users, add configurable setting, etc. (I want to reiterate, all of these are good things, but we'll start learning more and sooner by shipping.)

@lbry-bot lbry-bot assigned BrannonKing and unassigned BrannonKing Feb 3, 2020
@BrannonKing
Copy link
Member Author

I've added ffmpeg_status to the status() method on the daemon. It includes fields: availabe, which, and analyze_audio_volume.

@eukreign eukreign changed the title Video transcoding added video file validation and optimization when publishing streams (using --validate_file and --optimize_file arguments) Feb 4, 2020
@eukreign eukreign merged commit d745c04 into master Feb 4, 2020
@eukreign eukreign deleted the add_video_transcoding branch February 4, 2020 03:29
@eukreign eukreign added area: files type: new feature New functionality that does not exist yet labels Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: files type: new feature New functionality that does not exist yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize videos for quickstart (moov atom)
4 participants