Works for me with latest master.

Might be file specific ?

hm... i'll provide a file at weekend.

or, can it be libvpx version issue? 
apt-cache policy libvpx1
libvpx1:
  Installiert:           1.3.0-2
  Installationskandidat: 1.3.0-2
  Versionstabelle:
 *** 1.3.0-2 0
        500 http://de.archive.ubuntu.com/ubuntu/ trusty/main amd64 Packages
        100 /var/lib/dpkg/status

Works with v1.3.0 and v1.4.0.

I did some more tests. If i use only part of this file, then dual pass is working. If complete file is used, then dual pass will fail. I assume multipass.cache has some corruptions.


stream.dump (complete video dump) https://www.dropbox.com/s/rzn444d9sz8k8pa/stream.dump?dl=0
multipass.cache (probably corrupt file created by fist pass) https://www.dropbox.com/s/uprgegvlaaqrd3i/multipass.cache?dl=0

Hid Vincent,

did you downloaded sample file? May i remove it from dropbox, i need some place :)

Seems I can't download from dropbox without making an account anymore, so delete it for now as I'm not sure when I'd get more time to debug this atm.

I found a patch coused this regression. See https://bugzilla.gnome.org/show_bug.cgi?id=726329#c23

I place my comment here too:
this patch (Bug 726329) introduced an regression on two pass encoding. Some DVDs will cause caps renegotiation just at the end of stream and as result corrupted multipass cache.
The issue looks like fallowing:
1. start of stream
2. frames
.........
5. caps reinit.
6. frames
6. end of stream.

Since reinit will send NULL frame image to the encoder, it will create EOS header for the multipass cache file. Next frame will reinit multipass file and actual end of stream will create one more header with EOS and wrong count of frames.

So how can we prevent this? Will the renegotiation actually change the caps, if so how are the caps different before and after?

caps are sort of different:
/GstPipeline:pipeline0/GstMpeg2dec:mpeg2dec0.GstPad:src: caps = "video/x-raw\,\ format\=\(string\)I420\,\ width\=\(int\)720\,\ height\=\(int\)576\,\ pixel-aspect-ratio\=\(fraction\)256/135\,\ interlace-mode\=\(string\)mixed\,\ chroma-site\=\(string\)mpeg2\,\ colorimetry\=\(string\)2:4:8:3\,\ framerate\=\(fraction\)25/1"

vs

/GstPipeline:pipeline0/GstMpeg2dec:mpeg2dec0.GstPad:src: caps = "video/x-raw\,\ format\=\(string\)I420\,\ width\=\(int\)720\,\ height\=\(int\)576\,\ pixel-aspect-ratio\=\(fraction\)256/135\,\ interlace-mode\=\(string\)mixed\,\ chroma-site\=\(string\)mpeg2\,\ colorimetry\=\(string\)2:4:5:4\,\ framerate\=\(fraction\)25/1"

The only difference is colorimetry\=\(string\)2:4:5:4.
hm.. what is the best way to solve it? I think about some variants:
- don't reinit on caps which we don't support.
- don't reinit on multipass mode
- use counter for files suffix for each reinit: multipass.cache.0 ... multipass.cache.2 and so on
- other ideas?

Also first of all, try to understand *why* the colorimetry is changing :)

I assume because content was changed to plain black frames.

That's not enough, I wouldn't be surprised if the caps change is actually wrong.


But your idea to use separate multipass.cache.X files sounds like the best so far. Want to provide a patch?

Sure. Can you please help me to create test case? Currently i need to encode 4GB to get this point.

You could create a pipeline with videotestsrc ! capsfilter ! vp8enc ! ... and then at some point change e.g. the resolution in the capsfilter.

Created attachment 307101 [details]
test file with different caps.

Created attachment 307132 [details] [review]
patch v1

So, how about this patch?

Review of attachment 307132 [details] [review]:

::: ext/vpx/gstvp8enc.c
@@ +761,3 @@
   gst_vp8_enc->cfg.g_pass = DEFAULT_MULTIPASS_MODE;
+  gst_vp8_enc->multipass_cache_prefix = g_strdup (DEFAULT_MULTIPASS_CACHE_FILE);
+  gst_vp8_enc->multipass_cache_prefix = NULL;

This code seems to dup a string, then set it to NULL. This seems like a bug (and a leak).

@@ +805,3 @@
 
+  g_free (gst_vp8_enc->multipass_cache_prefix);
+  gst_vp8_enc->multipass_cache_prefix = NULL;

Note for future (you are following the style so not your fault) finalize() is never called twice, so there is no need to reset the value to NULL or zero.

@@ +913,3 @@
+      if (gst_vp8_enc->multipass_cache_prefix)
+        g_free (gst_vp8_enc->multipass_cache_prefix);
+      gst_vp8_enc->multipass_cache_prefix = g_value_dup_string (value);

So are you changing the meaning of an existing property ? This seems like an API break.

(In reply to Nicolas Dufresne (stormer) from comment #19)
> Review of attachment 307132 [details] [review] [review]:
> 
> ::: ext/vpx/gstvp8enc.c
> @@ +761,3 @@
>    gst_vp8_enc->cfg.g_pass = DEFAULT_MULTIPASS_MODE;
> +  gst_vp8_enc->multipass_cache_prefix = g_strdup
> (DEFAULT_MULTIPASS_CACHE_FILE);
> +  gst_vp8_enc->multipass_cache_prefix = NULL;
> 
> This code seems to dup a string, then set it to NULL. This seems like a bug
> (and a leak).

oops, you right.

> 
> @@ +805,3 @@
>  
> +  g_free (gst_vp8_enc->multipass_cache_prefix);
> +  gst_vp8_enc->multipass_cache_prefix = NULL;
> 
> Note for future (you are following the style so not your fault) finalize()
> is never called twice, so there is no need to reset the value to NULL or
> zero.

ok

> @@ +913,3 @@
> +      if (gst_vp8_enc->multipass_cache_prefix)
> +        g_free (gst_vp8_enc->multipass_cache_prefix);
> +      gst_vp8_enc->multipass_cache_prefix = g_value_dup_string (value);
> 
> So are you changing the meaning of an existing property ? This seems like an
> API break.

yes, i was thinking about it too. Any idea how to solve it without braking the api?

Created attachment 307193 [details] [review]
patch v2

Comment on attachment 307193 [details] [review]
patch v2

Not sure what to do about the subtle change in how the property works. Perhaps just make it so that the first file created is the filename specified, and only for indices > 0 you add the .N suffix to the filename. I suppose the app is supposed to clean those up, but it should create those in a temp dir anyway, so don't think this is a huge problem. The behaviour should maybe be documented for the property, so app developers can know about this without reading the code.

Created attachment 309071 [details] [review]
patch v3

New version. As suggested cache files will be named:
cache
cache.1
cache.2
... and so on
What do you mean about documentation? Should it be in same patch?

Doesn't really matter, same patch would be good IMHO. Just add a short note to tell application developers that there might be multiple cache files, under what conditions, and what the naming scheme will be.

Created attachment 309075 [details] [review]
patch v4

How about this?

Ok, let's get this in then:

commit abc33c928fc39ab58ec7f8f3784b72b4266242ae
Author: Oleksij Rempel <linux@rempel-privat.de>
Date:   Thu Jul 9 09:51:26 2015 +0200

    vp8enc: provide support for multiple pass cache files
    
    Some files may provide different caps insight of one stream. Since vp8enc
    support caps reinit, we should support cache reinit too.
    If more then file cache file will be created, the naming will be:
    cache
    cache.1
    cache.2
    ...
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747728

Question: don't we need to reset the cache_idx to 0 somewhere?

When doing multi-pass encoding, do you create a new encoder element? I think it needs to be reset to 0 in _stop() or so, so that if the same pipeline/element is re-used for the second pass, it starts reading the right cache files. If you confirm I'll fix it up.

Hm... yes you right. If it can be called without gst_vp8_enc_finalize, then we need it on gst_vp8_enc_stop too.

The same change should be applied to vp9enc :)


stop() should always free all stream-specific information, yes.

commit 98527a6ea22198db575822dc8201cacea1d5d8bb
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sat Aug 15 11:12:05 2015 +0100

    vp8enc, vp9enc: reset multipass file index when stopping encoder
    
    Fixes multipass encoding when re-using the same element/pipeline
    for subsequent encoding runs.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747728

commit 0e10e9295376282036fef0a2129101d7fae9bb94
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Sat Aug 15 11:09:42 2015 +0100

    vp9enc: provide support for multiple pass cache files
    
    Some files may provide different caps insight of one stream. Since
    vp9enc support caps reinit, we should support cache reinit too.
    If more then file cache file will be created, the naming will be:
    cache cache.1 cache.2 ...
    
    Based on patch by: Oleksij Rempel <linux@rempel-privat.de>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747728

Great! Thank you!

*** Bug 740002 has been marked as a duplicate of this bug. ***

*** Bug 719799 has been marked as a duplicate of this bug. ***