Bug Report about mp4 module

[Xietong LU]

The ngx_http_mp4_update_stsc_atom function has some bug.

[maxim]

Probably. But could you please provide more information about the bug you discovered?

[Xietong LU]

Replying to maxim:

Probably. But could you please provide more information about the bug you discovered?

I think there are two bugs and I just found out how to fix one of them.
In line 2491, whenever trak->chunk_samples is true, the variable entries should increase 1.
So entries++; should be added.
I didn't test it yet because I can't compile nginx with my computer.

[Maxim Dounin]

Could you please instead provide a description of the problem you are seeing, and steps to reproduce it? I think you're right and something like this

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2500,6 +2500,8 @@ found:
 
     if (trak->chunk_samples) {
 
+        entries++;
+
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
         ngx_mp4_set_32value(first->samples, samples - trak->chunk_samples);

is indeed needed to correctly handle non-keyframe seeks, but it would be good to see the actual problem before trying to fix it.

[Xietong LU]

Replying to Maxim Dounin:

Could you please instead provide a description of the problem you are seeing, and steps to reproduce it? I think you're right and something like this

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2500,6 +2500,8 @@ found:
 
     if (trak->chunk_samples) {
 
+        entries++;
+
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
         ngx_mp4_set_32value(first->samples, samples - trak->chunk_samples);

is indeed needed to correctly handle non-keyframe seeks, but it would be good to see the actual problem before trying to fix it.

I use an mp4 file for testing. Inside the sample to chunk box, the original entries are

Entry{firstChunk=1, samplePerChunk=13, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=12, sampleDescriptionIndex=1}
Entry{firstChunk=167, samplePerChunk=9, sampleDescriptionIndex=1}

Then I use wget with different start time to download the regenerated mp4 files. When the start time is 0.1, where the start sample is 2nd one, the new entries are

Entry{firstChunk=1, samplePerChunk=11, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=13, sampleDescriptionIndex=1}
Entry{firstChunk=2, samplePerChunk=12, sampleDescriptionIndex=1}

(I'm using the mp4 parser from the website: ​http://code.google.com/p/mp4parser/)
Other information is as follow.
type: stsc
size: 64
version: 0
entries: 3

You can find 2 bugs here.

1. Entry{firstChunk=167, samplePerChunk=9, sampleDescriptionIndex=1} is missing. Actually, this entry is inside the file from the size of this box, but the number of entries is 3. Then the parser didn't show it out.
2. Entry{firstChunk=2, samplePerChunk=13, sampleDescriptionIndex=1} should be deleted. It happened when the start sample is located in the last chunk specified by an entry. For example, when the start sample are from 1983 - 1993, similar problem will happen again.

My English is poor and I can't express it very clearly.

[Xietong LU]

I think the second problem can be solved by this way

+       if (trak->chunk_samples&& (start_sample/samples == next_chunk - chunk)){
+	   ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+	}
        else if (trak->chunk_samples) {

		entries++; 
		...

[Maxim Dounin]

Could you please provide the mp4 file in question for testing?

[Xietong LU]

Replying to Maxim Dounin:

Could you please provide the mp4 file in question for testing?

The original file.
​https://docs.google.com/open?id=0B3Wt0yFGIKL1M21fTDEtM2lUR1k
The output file.
​https://docs.google.com/open?id=0B3Wt0yFGIKL1WE16Sm5hb19oX0k

[Maxim Dounin]

Ok, thanks. The following patch seems to resolve both issues:

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2498,7 +2498,13 @@ found:
 
     ngx_mp4_set_32value(entry->chunk, 1);
 
-    if (trak->chunk_samples) {
+    if (trak->chunk_samples && next_chunk - trak->start_chunk == 2) {
+
+        /* last chunk in the entry */
+
+        ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+
+    } else if (trak->chunk_samples) {
 
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
@@ -2514,6 +2520,7 @@ found:
 
         ngx_mp4_set_32value(entry->chunk, 2);
 
+        entries++;
         atom_size += sizeof(ngx_mp4_stsc_entry_t);
     }
 

Is it looks ok for you?

[Xietong LU]

Replying to Maxim Dounin:

Ok, thanks. The following patch seems to resolve both issues:

--- a/src/http/modules/ngx_http_mp4_module.c
+++ b/src/http/modules/ngx_http_mp4_module.c
@@ -2498,7 +2498,13 @@ found:
 
     ngx_mp4_set_32value(entry->chunk, 1);
 
-    if (trak->chunk_samples) {
+    if (trak->chunk_samples && next_chunk - trak->start_chunk == 2) {
+
+        /* last chunk in the entry */
+
+        ngx_mp4_set_32value(entry->samples, samples - trak->chunk_samples);
+
+    } else if (trak->chunk_samples) {
 
         first = &trak->stsc_chunk_entry;
         ngx_mp4_set_32value(first->chunk, 1);
@@ -2514,6 +2520,7 @@ found:
 
         ngx_mp4_set_32value(entry->chunk, 2);
 
+        entries++;
         atom_size += sizeof(ngx_mp4_stsc_entry_t);
     }
 

Is it looks ok for you?

yes. It looks ok.

[Maxim Dounin]

In [4688/nginx]:

(The changeset message doesn't reference this ticket)

[Maxim Dounin]

Fix committed, thanks.

[Maxim Dounin]

In [4728/nginx]:

(The changeset message doesn't reference this ticket)