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

Video start_time not set to 0, black screen preview in QuickTime #644

Closed
jeffski opened this issue Mar 14, 2021 · 7 comments
Closed

Video start_time not set to 0, black screen preview in QuickTime #644

jeffski opened this issue Mar 14, 2021 · 7 comments
Labels
stale This issue has not had any activity in 90 days :(

Comments

@jeffski
Copy link
Contributor

jeffski commented Mar 14, 2021

It seems that when rendering video, the very first frame is not written at start_time 0 but instead at 0.04 (at 25 fps, it skips one frame).

This might not seem like a big problem and I think most viewers/players get around this but QuickTime players (including iPhone) shows the first frame as black instead of the first frame of the video. I think this affects preview thumbnails on iPhones too.

QuickTime shows a black screen when the video loads:
QT-player-black

To fix this I can run a simple ffmpeg command, which just copies the video but it resets the start_time setting to 0 and the preview/first frame displays correctly:

ffmpeg -i original.mp4 -c copy fixed.mp4

This is the expected result with the first frame used as a preview/poster image:
QT-player-ok

To recreate this issue:

Run the following code to render a video clip:

	Clip clip(new FFmpegReader("sample.mp4"));
	clip.Position(0.0);
	clip.Layer(1);
	clip.Start(0.0);
	clip.End(10);

	Timeline timeline(1280, 720, Fraction(25,1), 44100, 2, LAYOUT_STEREO);
	timeline.AddClip(&clip);
	timeline.Open();

	FFmpegWriter writer("original.mp4");
	writer.SetAudioOptions(true, "aac", 44100, 2, LAYOUT_STEREO, 128000);
	writer.SetVideoOptions(true, "libx264", Fraction(25, 1), 1280, 720, Fraction(1,1), false, false, 0);
	writer.PrepareStreams();
	writer.SetOption(VIDEO_STREAM, "crf", "23" );
	writer.WriteHeader();
	writer.Open();
	writer.WriteFrame(&timeline, 0, 10 * 25);

	writer.Close();
	timeline.Close();

Check the start_time using ffprobe:
ffprobe -v error -select_streams v:0 -show_entries stream=start_time -of default=noprint_wrappers=1 original.mp4

You should get a value:
start_time=0.040000

To fix it:
ffmpeg -i original.mp4 -c copy fixed.mp4

Check the start_time using ffprobe:
ffprobe -v error -select_streams v:0 -show_entries stream=start_time -of default=noprint_wrappers=1 fixed.mp4

You should get a value:
start_time=0.000000

You can compare in QuickTime too.

Is this something that can be fixed in libopenshot rather than afterwards using ffmpeg? Or is there a setting I'm missing that will force the first frame to be written at 0.00000. Is this a bug or a design choice? I think somewhere in the code it must be skipping frame 0 and starts at frame 1 (0.04 seconds at 25 fps).

Any feedback on this would be appreciated. If it's a bug we can probably dedicate some resources to fixing this.

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 15, 2021

	writer.WriteFrame(&timeline, 0, 10 * 25);

That should be writer.WriteFrame(&timeline, 1, 10*25);, since OpenShot's frame numbers are 1-based, and WriteFrame works in frame numbers, not time positions. I suspect that may be the source of the issue, though it's arguably still a bug that it would accept a 0-based frame number and write a blank frame as a result.

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 15, 2021

Interestingly, the loop in WriteFrame passes the start value straight through to reader->GetFrame(), which only then corrects the frame number:

std::shared_ptr<Frame> FFmpegReader::GetFrame(int64_t requested_frame) {
// Check for open reader (or throw exception)
if (!is_open)
throw ReaderClosed("The FFmpegReader is closed. Call Open() before calling this method.", path);
// Adjust for a requested frame that is too small or too large
if (requested_frame < 1)
requested_frame = 1;
if (requested_frame > info.video_length && is_duration_known)
requested_frame = info.video_length;

There's also either a bug or a documentation bug in FFmpegWriter, since based on this loop:

void FFmpegWriter::WriteFrame(ReaderBase *reader, int64_t start, int64_t length) {
ZmqLogger::Instance()->AppendDebugMethod("FFmpegWriter::WriteFrame (from Reader)", "start", start, "length", length);
// Loop through each frame (and encoded it)
for (int64_t number = start; number <= length; number++) {
// Get the frame
std::shared_ptr<Frame> f = reader->GetFrame(number);
// Encode frame
WriteFrame(f);
}
}

The third argument to WriteFrame isn't a length but an end_frame. Otherwise, the loop should be:

for (int64_t number = start; number < (start + length); number++) { 

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 15, 2021

Still not super clear on why you're getting a blank first frame / start time offset, since it seems like what should be happening is that the first frame ends up doubled (after the 0 frame request is adjusted to 1, the first time, and then frame 1 is requested again for the following frame).

I suppose it could be that FFmpeg is detecting the duplicate frame, and applying some sort of mitigation that drops the first and shifts the start time to the time index of the second.

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 15, 2021

Also, it seems incredibly dumb to me that the code would just forcibly adjust an out-of-bounds frame number, then proceed as if nothing's wrong — rather than, say, throwing an exception. Hard to see how that's really doing anyone any favors.

There's even an exception defined just for that purpose:

/// Exception for frames that are out of bounds.
class OutOfBoundsFrame : public ExceptionBase
{
public:
int64_t FrameRequested;
int64_t MaxFrames;
/**
* @brief Constructor
*
* @param message A message to accompany the exception
* @param frame_requested The out-of-bounds frame number requested
* @param max_frames The maximum available frame number
*/
OutOfBoundsFrame(std::string message, int64_t frame_requested, int64_t max_frames)
: ExceptionBase(message), FrameRequested(frame_requested), MaxFrames(max_frames) { }
virtual ~OutOfBoundsFrame() noexcept {}
};

Currently OutOfBoundsFrame is only thrown at two places in the code:

  1. In FrameMapper:
    // Check if frame number is valid
    if(TargetFrameNumber < 1 || frames.size() == 0)
    // frame too small, return error
    throw OutOfBoundsFrame("An invalid frame was requested.", TargetFrameNumber, frames.size());
  2. In FFmpegReader(!??!):
    // Allocate an AVFrame structure
    pFrameRGB = AV_ALLOCATE_FRAME();
    if (pFrameRGB == NULL)
    throw OutOfBoundsFrame("Convert Image Broke!", current_frame, video_length);

...And I'm not even sure that second one is really a valid use of the OutOfBoundsFrame exception in particular.

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 15, 2021

...And I'm not even sure that second one is really a valid use of the OutOfBoundsFrame exception in particular.

Actually, scratch that, I'm sure it's not. When built with any recent FFmpeg version, the AV_ALLOCATE_FRAME() macro just translates to a bare av_frame_alloc(). If it returns nullptr then the only reasonable exception to throw is OutOfMemory.

@jeffski
Copy link
Contributor Author

jeffski commented Mar 15, 2021

Thanks @ferdnyc for your detailed analysis as always. I tried with:

writer.WriteFrame(&timeline, 1, 10 * 25);

But still got the same start_time of 0.040000

I also tried different start frame numbers, i.e.:

writer.WriteFrame(&timeline, 10, 9 * 25);

And still get the same start_time.

We're you able to recreate this issue?

@stale
Copy link

stale bot commented Jun 14, 2021

Thank you so much for submitting an issue to help improve OpenShot Video Editor. We are sorry about this, but this particular issue has gone unnoticed for quite some time. To help keep the OpenShot GitHub Issue Tracker organized and focused, we must ensure that every issue is correctly labelled and triaged, to get the proper attention.

This issue will be closed, as it meets the following criteria:

  • No activity in the past 90 days
  • No one is assigned to this issue

We'd like to ask you to help us out and determine whether this issue should be reopened.

  • If this issue is reporting a bug, please can you attempt to reproduce on the latest daily build to help us to understand whether the bug still needs our attention.
  • If this issue is proposing a new feature, please can you verify whether the feature proposal is still relevant.

Thanks again for your help!

@stale stale bot added the stale This issue has not had any activity in 90 days :( label Jun 14, 2021
@stale stale bot closed this as completed Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This issue has not had any activity in 90 days :(
Projects
None yet
Development

No branches or pull requests

2 participants