[vlc-devel] [RFC] demux/mp4: fix 17584: signal error if ReadBox_default fails

[Filip Roséen]

Hi Francois,

I appreciate you taking time to answer my questions, even though it
has now become evident that I do not agree with what you were
referring to as a regression (missing debug data).

On 2016-11-04 12:58, Francois Cartegnie wrote:

> Le 04/11/2016 à 12:07, Filip Roséen a écrit :
> > Sure, there might be unknown boxes which we cannot handle - but I
> > still fail to see how the patch would lead to a regression related to
> > how those boxes are handled.
> 
> I'm allowed to check if a box exists or not, even if it's unknown.
> Use cases I can think about: encryption test. We don't decode it, but we
> want to check if it applies.
> 
> > From my understanding other boxes can signal errors in the same way as
> > `MP4_ReadBox_default` would if the patch is applied, so I must be
> > missing something.
> > 
> >>  |   |   + udta size 83 offset 600
> >>  |   |   |   + creq size 36 offset 608
> >>  |   |   |   + cenc size 39 offset 644 (????)
> > 
> > Could you point me in the direction of a failure related to the patch
> > so that I properly understand why such thing should not be done (not
> > now, nor in the future)?
> 
> I'll send a patch to remove every structured debug output from mkv.
> I hope you won't be too annoyed to debug later issues.

I'd argue that this is a flaw in the code that handles the box-dumping
(and other debug logs), not the return-value of `MP4_ReadBox_default`.

I also do not see the relevance of even mentioning removing structured
output from some unrelated demuxer.

------------------------------------------------------------------------

Why I do not agree that the *"problem"* is specific to the patch
------------------------------------------------------------------------

The structured debug output will **not** contain boxes that fail to
parse in either case, even if this is a known box; so what you are
describing is, in my opinion, not specific to the patch we are
currently discussing.

With that said; rejecting the patch by saying that it is wrong due to
a problem that the implementation already suffers from is in my
opinion a *strawman* in terms of argumentation.

If the structured debug output should include boxes that fails to
parse, the implementation should be changed to include this data (not
treat an unknown box as if it was parsed successfully, in my opinion).

------------------------------------------------------------------------


Best Regards,\
Filip

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161104/80b4a691/attachment.html>